Creating check parameter class instead of repetitive null checks

Question

What I'm trying to accomplish is to have a error checking class mostly to check if parameters that are pass null but also data structures within class are not null. So my thought is a clean static method call to pass these variables and verify if it's valid, and if so, return a boolean.

For example, my code is seems to have a lot of repetitive checking. Yes, I know it might be too much, but I been ask to do so. So it's out of my control whether it's too much but I think there's some merit to it. But how should I got about making a class for this?

public class RAMUserDAO implements GenericDAO<User, String, Boolean>
{
    private static Map<String, User> activeUserMap = new ConcurrentHashMap<String, User>();
    private static Map<String, User> banUserMap = new ConcurrentHashMap<String, User>();

    public RAMUserDAO()
    {

    }

    @Override
    public boolean insert(User user, Boolean active) throws NotFoundException
        {
            if (user == null || user.getUserName() == null
                    || !(user instanceof User))
                throw new NotFoundException("RAMUserDAO insert");

            if (active)
            {
                if (activeUserMap.get(user.getUserName()) != null)
                    throw new NotFoundException(
                            "RAMUserDAO insert active User Exist");

                activeUserMap.put(user.getUserName(), user);
                LOG.SYSTEM.info("User: " + user.getUserName()
                        + "has been added RAMUserDAO");
                return true;
            }
            else
            {
                if (banUserMap.get(user.getUserName()) != null)
                    throw new NotFoundException(
                            "RAMUserDAO insert ban User Exist NULL");

                banUserMap.put(user.getUserName(), user);
                LOG.SYSTEM.info("User: " + user.getUserName()
                        + "has been banned RAMUserDAO");
                return true;
            }
        }

    @Override
    public boolean update(User user, Boolean active) throws NotFoundException
        {
            if (!isUser(user, active))
                throw new NotFoundException("User update RAMUserDAO");

            if (active)
            {
                activeUserMap.put(user.getUserName(), user);
                LOG.SYSTEM.info("User: " + user.getUserName()
                        + "has been updated RAMUserDAO");
                return true;
            }
            else
            {
                banUserMap.put(user.getUserName(), user);
                LOG.SYSTEM.info("Ban User: " + user.getUserName()
                        + "has been updated RAMUserDAO");
                return true;
            }
        }

    @Override
    public boolean delete(User user, Boolean active) throws NotFoundException
        {
            if (!isUser(user, active))
                throw new NotFoundException("User delete RAMUserDAO");

            if (active)
            {
                activeUserMap.remove(user.getUserName());
                LOG.SYSTEM.info("User: " + user.getUserName()
                        + "has been removed RAMUserDAO");
                return true;
            }
            else
            {
                banUserMap.remove(user.getUserName());
                LOG.SYSTEM.info("Ban User: " + user.getUserName()
                        + "has been removed RAMUserDAO");
                return true;
            }
        }

    @Override
    public User findByKey(String key, Boolean active) throws NotFoundException
        {
            if (!isUser(key, active))
                throw new NotFoundException("User findByKey RAMUserDAO");

            if (active)
            {
                return activeUserMap.get(key);
            }
            else
            {
                return banUserMap.get(key);
            }

        }

    @Override
    public User findByValue(User user, Boolean active) throws NotFoundException
        {
            if (!isUser(user, active))
                throw new NotFoundException("User findByValue RAMUserDAO");

            if (active)
            {
                return activeUserMap.get(user.getUserName());
            }
            else
            {
                return banUserMap.get(user.getUserName());
            }

        }

    private boolean isUser(String userName, Boolean active)
            throws NotFoundException
        {
            if (userName == null)
                throw new NotFoundException("RAMUserDAO isUser string null");
            if (active)
            {
                // LOG.CONSOLE.debug(activeUserMap);
                // LOG.CONSOLE.debug(activeUserMap.get(userName));
                if (activeUserMap == null
                        || activeUserMap.get(userName) == null)
                    throw new NotFoundException("RAMUserDAO isUser");
            }
            else
            {
                if (banUserMap == null || banUserMap.get(userName) == null)
                    throw new NotFoundException("RAMUserDAO isUser");
            }

            return true;
        }

    private boolean isUser(User user, Boolean active) throws NotFoundException
        {
            if (user == null || user.getUserName() == null
                    || !(user instanceof User))
                throw new NotFoundException("RAMUserDAO isUser");

            if (active)
            {
                if (activeUserMap == null
                        || activeUserMap.get(user.getUserName()) == null)
                    throw new NotFoundException("RAMUserDAO isUser");
            }
            else
            {
                if (banUserMap == null
                        || banUserMap.get(user.getUserName()) == null)
                    throw new NotFoundException("RAMUserDAO isUser");
            }

            return true;
        }

    /**
     * @return the userList
     * @throws NotFoundException
     */
    public Map<String, User> getMap() throws NotFoundException
        {
            if (activeUserMap == null)
                throw new NotFoundException("getUserList RAMUserDAO null");

            return activeUserMap;
        }

    /**
     * @return the banList
     * @throws NotFoundException
     */
    public static Map<String, User> getBanList() throws NotFoundException
        {
            if (banUserMap == null) throw new NotFoundException();
            return banUserMap;
        }

}

Answer 1

There are several things that can be improved here.

• You create activeUserMap and banUserMap and have no methods that set them, so those can be made final and there is no need to check if they are null.
• You have methods that return booleans (presumably to indicate success/failure) but they always either return true or throw an exception. You should either return void from these methods or return false in those places where you are throwing exceptions.
• Why are you checking !(user instanceof User) when you have already typed the object as User?
• You have a method named getBanList that returns a Map. That seems odd.
• You are returning modifiable internal collections to the caller. You should wrap them with Collections.unmodifiableMap() to make sure that your internal state doesn't get modified elsewhere.

Other posters have made great recommendations as well, which I won't bother repeating.

Answer 2

Class design

Your class and interface design is not good. All the interface methods take a boolean parameter indicating whether you should operate on the map of active users or the map of banned users. A more natural interface would have these methods instead:

• addUser, updateUser, deleteUser, userExists
• banUser, unbanUser, isUserBanned

These methods should be independent, each have its own well-defined responsibility, and NO if-else branches to decide which internal map to operate on.

Exception messages

These kind of messages in exceptions are unnatural pointless:

public User findByKey(String username, Boolean active) throws NotFoundException {
    throw new NotFoundException("User findByKey RAMUserDAO");

You put in the message the method name and the class name where this happens. But the stack trace will already include that information. It would be better this way:

public User findByKey(String username, Boolean active) throws NoSuchUserException {
    throw new NoSuchUserException("No such user: " + username);

The insert, update, delete methods

There are several problems here:

public boolean insert(User user, Boolean active) throws NotFoundException {
    if (user == null || user.getUserName() == null
            || !(user instanceof User))
        throw new NotFoundException("RAMUserDAO insert");

First of all, since the method signature defines user as type User, the instanceof there is completely pointless. Remove it.

It's very strange for an insert method to throw NotFoundException. You're trying to insert something, and unexpectedly, something is not found? That doesn't make sense. I would expect unique key violation or other validation exception but not "something not found".

And in these particular null checks, you're really checking the inputs that you received from the caller. So it would be better to throw new IllegalArgumentException("user or user.getUserName is null"); or something like that.

Next, you have this kind of code:

if (active) {
    // ...
    return true;
} else {
    // ...
    return true;
}

In both branches of the if, you return true at the end. So you can actually move the return true line out of the if.

Also is there any point to insert and update? I should just combine?

There is a point only if you explicitly what such behavior that insert throws an exception if an entry already exists. If you don't really need such behavior then you don't need the insert method, you can use just update. Note that Map.put works the same way whether the key exists or not: if the key doesn't exist, it inserts it, if it exists, it updates it.

The isUser methods

In the isUser methods, you're checking for activeUserMap == null is non-sense: it's never null, because this map is initialized when the class is created. The same goes for the banUserMap == null check.

The two isUser methods do almost the same thing. You are unnecessarily duplicating code. The isUser(User user, Boolean active) could reuse the other one:

private boolean isUser(User user, Boolean active) throws NotFoundException {
    // using this clever method from David Harkness' answer
    ensureValidUser(user);
    return isUser(user.getUserName(), active);
}

The findByKey, findByValue methods

The isUser methods never actually return false: they either return true or they throw an exception. They could just as well be void. You don't need to check their return values, just let them throw an exception. Effectively, rewrite as David Harkness suggested as ensureValidUser.

getMap

Once again, the unnecessary null check, when the method could have been simply:

public Map<String, User> getMap() {
    return activeUserMap;
}

Answer 3

Check out Guava's Preconditions helpers for some ideas, specifically checkNotNull. But first let's look at what you have in the first method:

public boolean insert(User user, Boolean active) throws NotFoundException {
    if (user == null || user.getUserName() == null
            || !(user instanceof User))
        throw new NotFoundException("RAMUserDAO insert");
    ...
}

1. user must be an instance of User for the calling code to compile, so you can drop the third check.

2. I highly recommend using braces even for one-line blocks. It's very easy to add a second line thinking it is part of the block and creating a bug.

   if (x % 2 == 0)
       x += 5;
       x /= 3;  // bug! this always applies
   

You'll need to decide if you want to combine some generic not-null checks with class-specific ones (name is not null) or use solely class-specific checks and duplicate the generic checks for each.

One simple refactoring of the above is to extract the checks to ensureValidUser:

public boolean insert(User user, Boolean active) throws NotFoundException {
    ensureValidUser(user);
    ...
}

private void ensureValidUser(User user) {
    if (user == null || user.getUserName() == null) {
        throw new NotFoundException("User and name must be non-null");
    }
}

If these are the only checks you need, you can give it a more descriptive name such as ensureUserHasName.