2

I had the following code, which is responsible for assigning roles and groups to users.

private void override(List<Assignment> assignments) {
    removeAll();
    addMultiple(assignments);
}

protected void removeAll() {
    removeAllRoles();
    removeAllGroups();
}

private void removeAllGroups() {
    Iterator<String> userGroups = user.getParentGroups(false);
    while (userGroups.hasNext()) {
        UMHelper.removeUserFromGroup(user.getUniqueID(), userGroups.next());
    }
}

private void addMultiple(List<Assignment> assignments) {
    for (Assignment assignment : assignments) {
        add(assignment);
    }
}

public static void addUserToGroup(String userId, String groupId) {
    try {
            SimpleLogger.log(Severity.INFO, Category.APPLICATIONS, loc, null, "Trying to add user " + userId + " to group " + groupId);
            groupFactory.addUserToGroup(userId, groupId);
            SimpleLogger.log(Severity.INFO, Category.APPLICATIONS, loc, null, "Success");
        } catch (UMException e) {
            SimpleLogger.traceThrowable(Severity.ERROR, loc, "Addition failed", e);
        }
}

I hope the logic is pretty clear. Most of the code is iteration over collections. Adding a user to role or group can cause exception, which I report in log.

Since I find it not good to suppress exceptions and because a client calling override method should know the result of assigment, I rewrote the code adding exception handling. The execution should continue, even if some assignments failed.

private void override(List<Assignment> assignments) {
    SimpleLogger.log(Severity.INFO, Category.APPLICATIONS, loc, null, "Override was started with " + assignments.size() + " assignments");

    try {
        removeAll();
    } catch (UMException e) {
        SimpleLogger.log(Severity.INFO, Category.APPLICATIONS, loc, null, "Removing all existing elements failed");
    }

    try {
        addMultiple(assignments);
    } catch (UMException e) {
        SimpleLogger.log(Severity.INFO, Category.APPLICATIONS, loc, null, "Adding new elements failed");
    }
}

protected void removeAll() throws UMException {
    boolean successfulRemoval = true;

    try {
        removeAllRoles();
    } catch (UMException e) {
        successfulRemoval = false;
    }

    try {
        removeAllGroups();
    } catch (UMException e) {
        successfulRemoval = false;
    }

    if (!successfulRemoval){
        throw new UMException("Not all user elements could be removed");
    }
}

private void removeAllGroups() throws UMException {
    boolean removeSuccessful = true;

    Iterator<String> userGroups = user.getParentGroups(false);
    while (userGroups.hasNext()) {
        try {
            UMHelper.removeUserFromGroup(user.getUniqueID(), userGroups.next());
        } catch (UMException e) {
            removeSuccessful = false;
        }
    }

    if (!removeSuccessful) {
        throw new UMException("Not all user groups could be removed");
    }
}

private void addMultiple(List<Assignment> assignments) throws UMException {
    boolean additionSuccessful = true;

    for (Assignment assignment : assignments) {
        try {
            add(assignment);
        } catch (UMException e) {
            additionSuccessful = false;
        }
    }

    if (!additionSuccessful) {
        throw new UMException("Addition of new rights failed");
    }
}

public static void addUserToGroup(String userId, String groupId) throws UMException {
        SimpleLogger.log(Severity.INFO, Category.APPLICATIONS, loc, null, "Trying to add user " + userId + " to group " + groupId);
        groupFactory.addUserToGroup(userId, groupId);
        SimpleLogger.log(Severity.INFO, Category.APPLICATIONS, loc, null, "Success");
}

Now the code got 3 times bigger and not as clear as it was. If I just had to stop execution after first assignment failed, the code would have been easier, but that's the requirement. Do I handle exceptions wrong or is there a way to improve the code?

Improve this question
5
  • 2
    It seems that you don't remove roles and group in a signle transaction, is that on purpose? Commented Apr 8, 2014 at 8:20
  • 1
    I think perceived clarity is a matter of getting-used-to here. The logic of your code actually is that every statement can fail, but the flow continues. That's exactly what I get from reading your second snippet. If you try to hide the exception handling somehow, I would get the wrong impression of what the code is doing. Commented Apr 8, 2014 at 8:22
  • Yes, I would be glad to do it, but SAP Portal APIs don't allow it or to be precise, allow it partially. There is an implicit commit after adding user to a group/role. Commented Apr 8, 2014 at 8:23
  • You are catching UMException, settings boolean flag and than throw new UMException. Your removeAll method is actually two-liner if you remove all that unnecessary "exception handling". Commented Apr 8, 2014 at 8:23
  • @OlegEstekhin, I have to continue executing the code. So if removeGroups failed, I want to try to remove roles. Commented Apr 8, 2014 at 8:28

2 Answers 2

Reset to default
2

In this scenario I would change all these methods from throwing exceptions to
returning a boolean value which indicates if they did their job successfully or not.
If you have control over these methods and can do this change, I think that's better
suited for your scenario.

Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

Hi, thank you. Yes, I thought about it, but the code would stay the same in this case, I would need to look if there was false returned for at least one assignment and set the whole operation to false.
So you want transactional behavior after all? I thought you wanted just the opposite. I am confused now. My answer and fge's answer are basically on the opposite banks.
No, I want clean code :) The whole problem is that I have to continue execution even if some assignments fails.
Then use my solution. Don't throw exceptions to start with. But if you need transactional behavior use fge's solution.
2

With a little code reorganization, that is, all removals/additions/etc are in a single transaction, the code can be made clearer, as in, for instance:

String failmsg = null;

// tx is some transaction object
tx.start();
try {
    failmsg = "user removal failed";
    tx.removeUsers();
    failmsg = "group removal failed";
    tx.removeGroups();
    failmsg = "new additions failed";
    tx.addNew();
    tx.commit();
} catch (UMException e) {
    tx.rollback();
    log(failmsg);
} finally {
    tx.close();
}
Improve this answer

5 Comments

Yes, if I had a choice to stop execution after first failure, I wouldn't have this question. But anyway +1 from me.
Is there really no way that you delve deeper into the API and achieve the above?
it's not actually about API. Bunch of assignments is sent to the system. It has sense to try to execute all of them and report those that fail. Let's say we have 10 failing assignments. If we stop after first one, we would have to execute the program 10 times, each time getting new error. To make a commit/rollback at the end is another question. We could try to assign everything and still make commit/rollback at the end. I hope I managed to explain it.
I thought about not throwing exceptions, but setting some assignmentStatus variable to indicate that there was an error in some assignment. But it's also more of a hack.
There are ways around that... You can design an interface/abstract class with chaining

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.