CODEWITHSUNDEEP
javaexception
Konstantin Milyutin
Konstantin Milyutin

Reputation: 12366

Exception handling makes code logic unclear

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?

Upvotes: 2

Views: 86

Answers (2)

fge
fge

Reputation: 121710

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();
}

Upvotes: 2

peter.petrov
peter.petrov

Reputation: 39457

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.

Upvotes: 2

Related Questions