-3

How do I transform this switch-case block into a good code?

private static void agentFieldContructor(Agent agent, String nodeName, String value) {

    switch (nodeName) {

        case "Description":
            agent.setDescription(value);
            break;

        case "Model":
            agent.setModel(value);
            break;

        (... +18)

    }

}

agent is an object and I'm filling it with the value parameter according to the nodeName specified.
Each nodeName refers to a different agent attribute, but I'm receiving it like a String and I cannot change this. I've searched some Design Patterns but couldn't find anything that could help me.

Silvio Mayolo
  • 62,821
  • 6
  • 74
  • 116
Marcus Rigonati
  • 25
  • 8
  • 5
    What makes you think a switch...case isn't good code? You have to work with what you have, and if you're receiving it as a String, a switch...case is as valid a method as any. – AntonH Dec 11 '17 at 18:42
  • Your switch isn't necessarily bad.... however, check out BeanUtils. https://commons.apache.org/proper/commons-beanutils/apidocs/org/apache/commons/beanutils/BeanUtils.html#setProperty-java.lang.Object-java.lang.String-java.lang.Object- – Lukas Bradley Dec 11 '17 at 18:44
  • 1
    What I learned in Object Oriented Languagens was that:If you have a giant if-else block, or switch-case, you're doing something wrong. – Marcus Rigonati Dec 11 '17 at 18:44
  • Just to be clear: you're trying to set properties on your agent instance by virtue of `value`? – Makoto Dec 11 '17 at 18:46
  • Switch case in and of itself is not necessarily bad code. In this particular case, you should write a proper constructor in the `Agent` class or require that the setters are called directly rather than through this method. – Code-Apprentice Dec 11 '17 at 18:46
  • @Makoto Exatly! – Marcus Rigonati Dec 11 '17 at 18:55

5 Answers5

1

If you are allowed to use reflection.

private static void agentFieldContructor(Agent agent, String nodeName, String value) {
    Method setMethod =  Agent.class.getMethod("set"+nodeName,String.class)
    setMethod.invoke(Agent,value)
}
halfer
  • 19,824
  • 17
  • 99
  • 186
Amakak
  • 31
  • 5
1

One compact way to accomplish this would be through reflection. You can get access to the setter method and invoke it with your value.

public static void agentFieldConstructor(Agent agent, String nodeName, String value) {
    try {
        agent.getClass().getDeclaredMethod("set" + nodeName, value.getClass()).invoke(agent, value);
    } catch (IllegalAccessException | InvocationTargetException | NoSuchMethodException e) {
        e.printStackTrace();
    }
}

Note that you're going to have to handle:

  • What happens if they insert an invalid node name
  • What happens if value is an unexpected value (like null)
  • Handling mixed case node names (right now, this works if the first letter is capitalized)
  • Handling non-standard node names (this would assume JavaBean naming conventions for the most part)

...all of which is an exercise for the reader.

Makoto
  • 104,088
  • 27
  • 192
  • 230
  • Yes, it's an answer to the OP's question. But compared with the switch-construct: it's MUCH slower, it's less readable, it moves some error checking from compile-time to run-time, IDE tools like Eclipse's "Call Hierarchy" don't see the implicit call of e.g. `setModel()`. So I'd recommend to stay away from reflection as far as possible. – Ralf Kleberhoff Dec 11 '17 at 19:37
  • @MarcusRigonati Note that even though this works, this is not an intended use of reflection. The fact that you are resorting to this implies that there are problems with the original design of Agent and the code which uses it. – Code-Apprentice Dec 11 '17 at 19:57
  • @Code-Apprentice: Given that the intended use of reflection is to allow metaprogramming like this, I'm not sure I agree with your assessment. I don't disagree that it's slow and that this is completely non-standard, but this is indeed a valid use case for reflection. – Makoto Dec 11 '17 at 20:07
  • @RalfKleberhoff: The OP finds themselves in a position where they can't exactly rearchitect how this object is built. What other approach would *you* recommend? – Makoto Dec 11 '17 at 20:07
  • @Makoto I cannot give another recommendation without more information. In particular, it would be helpful to know where `nodeName` comes from. – Code-Apprentice Dec 11 '17 at 20:11
  • I'd stay with the `switch` construct. – Ralf Kleberhoff Dec 11 '17 at 20:21
1

One more possibility to go without switch-case or reflection is to use complex enums. Here the enums are translators from the String key to the setter call:

public enum AgentFields {
    DESCRIPTION {
        @Override
        public void setInAgent(Agent agent, String value) {
            agent.setDescription(value);
        }
    },

    MODEL {
        @Override
        public void setInAgent(Agent agent, String value) {
            agent.setModel(value);
        }
    };

    public abstract void setInAgent(Agent agent, String value);

    // Call this method to set a named field's value
    public static void agentFieldSetter(Agent agent, String nodeName, String value) {
        AgentFields.valueOf(nodeName.toUpperCase()).setInAgent(agent, value);
    }

}

This declares an enum with one "value" per field, and each value carries an individual setter. The dispatching is done with the valueOf() method that all enums automatically supply. I decorated it with toUpperCase to allow for the usual upper-case enum value convention, and to make it case-insensitive.

Ralf Kleberhoff
  • 6,990
  • 1
  • 13
  • 7
  • I did think about using enum classes but I had one difficulty: I don't know how to use. I tried to learn but looks so complicated so I gave up for now. Anyway, I'm dealing with String, like if my String is "Model" I'll call setModel(), so Enum might not help so much. – Marcus Rigonati Dec 13 '17 at 15:52
  • Maybe you didn't recognize the agentFieldSetter() method which accepts a String as the field discriminator, so here the enum isn't the mapping key but the translator from String keys to setters. – Ralf Kleberhoff Dec 13 '17 at 16:26
  • Now I'm getting it, it could really help me, cause the other solutions is like a jerry-rig (I don't know how to say "gambiarra" in english). I'll make tests and comeback here if it help!! Thanks. – Marcus Rigonati Dec 13 '17 at 16:41
0

The problem here isn't with the switch-case statement per se. Rather it is with the design of the Agent class itself. First of all, Agent should have a proper constructor where you can pass in attribute values. If those values can change, then the user of Agent should call setters directly rather than with this helper function.

More importantly, a class with 20 attributes is a likely candidate for refactoring. Try to find relationships between subsets of these attributes which can be broken into separate classes.

Code-Apprentice
  • 81,660
  • 23
  • 145
  • 268
  • The constructor could be a good option, if I could get every value at once, but now I'm getting just one at a time. – Marcus Rigonati Dec 11 '17 at 18:53
  • The agent class is used to save in a Data Base, so I can't refactor this one. – Marcus Rigonati Dec 11 '17 at 18:54
  • "but now I'm getting just one at a time" Then call the setters directly. I do not see a good reason for the method you posted in the original question. Of course, my answer and comments are based only on what you have posted here. I have no idea where `nodeName` comes from, so using reflection as suggested earlier might be a better solution. – Code-Apprentice Dec 11 '17 at 19:54
  • Yes, I was calling the setters, my logic is: if nodeNade is "Model" then call setModel. But I have 20 attributes, so I was doing this in a swtich with 20 cases. Now I'm using reflection, I didn't knew about it, it's helping!! :) – Marcus Rigonati Dec 13 '17 at 15:46
  • @MarcusRigonati where does nodeName come from? – Code-Apprentice Dec 13 '17 at 15:50
0

I totally agree with comments. However, if you really do not want to use switch case and your method names and strings of cases are nearly same you can use reflection. Here is the solution which may help you.

Comteng
  • 11
  • 5