1
\$\begingroup\$

We have a class with 10 attributes (longs, Strings) the following method should change any attribute to any value of an object specified by the client,

It mimics the SQL update statement Update Users Set username = 'newValue' where id = 5

the user will provide the 4 variables id = 5 to find the object and username = newValue to change its username.

The method :

 public static void usersUpdate( String attributeToBeChanged, String newAttributeValue, String whereKey ,String whereValue) throws Exception{


        Method getter=getDeclaredMethodIgnoreCase(User.class,"get"+whereKey);

        HashMap<String,User> refToUsersCache= usersCache.getUsersCache(); // The objects are stored in this map as <username,User Object>
        User requiredUser = null;

        for(String user: refToUsersCache.keySet()) { // loop through the hashmap and call its getter to check its "ID" or any attribute
            if (getter.invoke(refToUsersCache.get(user)).equals(whereValue))  // getID() == "5" (in our example)
                requiredUser = refToUsersCache.get(user); // this is the required object
        }

        Method setter=getDeclaredMethodIgnoreCase(User.class,"set"+attributeToBeChanged);
        setter.invoke(requiredUser,newAttributeValue); // change the username = "newValue"

);

attributeToBeChanged = username

newAttributeValue= newValue

whereKey= id

whereValue = 5

\$\endgroup\$
0
\$\begingroup\$

The given code is only a snippet, so it's not really possible to compile and test it, but here's my review for what you posted:


Never say that a method throws Exception

The caller will either have to declare his method with throws Exception, or - in the worst case - he will wrap the call into a try-catch block like this:

try { ... } catch (Exception e) { /* ignore */ }

The problem is that this catches all exceptions. Even a NullPointerException.

The exception should be more specific - without leaking implementation details to the call site. There are different possible solutions for this. In any case, you then have to decide what should happen in each case. You could

  • wrap the exceptions into specific ones. Maybe an existing one, like an IllegalArgumentException

  • "ignore" the error, and just print a warning to the console

In both cases, the exception message or log output should contain the error message like

"Could not find method with name " + methodName

or

"Could not invoke method with name" + methodName

Stop the loop when you can stop it

Imagine there are 10000000 users. And imagine that the requiredUser is the first one in this collection. Then you will invoke the line

if (getter.invoke(refToUsersCache.get(user)).equals(whereValue))

unnecessarily 9999999 times. Changing this will also allow you to make the scope of the requiredUser variable smaller. And maybe it also forces you to think about what should happen when the user is not found. In your current implementation, this will cause a NullPointerException...


Avoid repeated map lookups

When iterating over the elements of a map, you often see this pattern:

for (Key key  : map.keySet()) {
    Value value = map.get(key);
    ... 
}

But in your case, you are not interested in the keys. You are just interested in the values, and thus, can iterate over them:

for (Value value : map.values()) {
    ... 
}

Even if you are interested in the exact key-value pair, you can iterate over the entries

for (Entry<Key, Value> entry  : map.entrySet()) {
    Key key = entry.getKey();
    Value value = entry.getValue();
    ... 
}

thus also avoiding the unnecessary lookups.


Program to the interface

Your code contained the line

HashMap<String, User> refToUsersCache = usersCache.getUsersCache();

Declaring the variable as a HashMap is not necessary. And in fact, it should not be possible. The getUsersCache() method should return a Map<String, User>, and you should thus only be able to declare your variable as a Map<String, User>.

Also see What does it mean to “program to an interface”? on stackoverflow.


Use // Inline comments judiciously

It's not clear whether you added these comments only for posting the code here. In any case, you should avoid appending such comments at the end of (already long) lines


Use sensible variable names

This is certainly controversial. But the line

HashMap<String, User> refToUsersCache = usersCache.getUsersCache();

makes me shudder. What type does the variable usersCache have? And whatever type it has, shouldn't getUsersCache() return an object that has a type that justifies the name usersCache? For me, this line looks as odd as a line like

Engine theEngine = engine.getCar();

Something just looks wrong there...


However, here is a sketch of how I made some of these changes locally. But note that this involves some guesses about the part of the code that you did not post, so take this with a grain of salt:

import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;

public class GetterSetterReflection
{
    public static void usersUpdate(String attributeToBeChanged,
        String newAttributeValue, String whereKey, String whereValue)
    {
        Map<String, User> usersCacheMap = usersCache.getUsersCache();
        for (User user : usersCacheMap.values())
        {
            Object userWhereValue = 
                invokeGetterUnchecked(User.class, user, whereKey);
            if (Objects.equals(userWhereValue, whereValue))
            {
                invokeSetterUnchecked(
                    User.class, user, attributeToBeChanged, newAttributeValue);
                return;
            }
        }
        // What should happen here?
        System.err.println("No user found where "+whereKey+" is "+whereValue);
    }

    private static Object invokeGetterUnchecked(
        Class<?> c, Object object, String propertyName)
    {
        try
        {
            String methodName = "get" + propertyName;
            Method getter = getDeclaredMethodIgnoreCase(c, methodName);
            return getter.invoke(object);
        }
        catch (NoSuchMethodException 
            | IllegalArgumentException
            | InvocationTargetException 
            | IllegalAccessException e)
        {
            throw new IllegalArgumentException(
                "Could not find or invoke getter for " + propertyName, e);
        }
    }

    private static void invokeSetterUnchecked(
        Class<?> c, Object object, String propertyName, Object value)
    {
        try
        {
            String methodName = "set" + propertyName;
            Method setter = getDeclaredMethodIgnoreCase(c, methodName);
            setter.invoke(object, value);
        }
        catch (NoSuchMethodException 
            | IllegalArgumentException
            | InvocationTargetException 
            | IllegalAccessException e)
        {
            throw new IllegalArgumentException(
                "Could not find or invoke setter for " + propertyName, e);
        }
    }

    //=========================================================================
    // Dummy code below this line

    private static Method getDeclaredMethodIgnoreCase(
        Class<?> c, String methodName) throws NoSuchMethodException
    {
        Method[] methods = c.getDeclaredMethods();
        for (Method method : methods)
        {
            String name = method.getName();
            if (name.equalsIgnoreCase(methodName))
            {
                return method;
            }
        }
        throw new NoSuchMethodException(
            "Method not found: " + methodName + " in " + c);
    }

    static class User
    {
    }
    static class UsersCacheHolder
    {
        HashMap<String,User> getUsersCache()
        {
            return null;
        }
    }
    private static UsersCacheHolder usersCache;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.