Creating check parameter class instead of repetitive null checks

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
{
private static Map activeUserMap = new ConcurrentHashMap();
private static Map banUserMap = new ConcurrentHashMap();

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

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 getMap() {
    return activeUserMap;
}

StackExchange Code Review Q#60197, answer score: 5