2

I have a switch statement that compares a String with set of String where each match calls a different method.

switch(((Operation) expr.getData()).getValue()){
        case "+":
            return add(expr.getNext());
        case "car":
            return car(expr.getNext());
        case "cdr":
            return cdr(expr.getNext());
        case "cons":
            return cons(expr.getNext(), expr.getNext().getNext());
        case "quote":
            return quote(expr.getNext());
        case "define":
            handleDefine(expr.getNext());
            break;
        default:
            return null;
        }

However, to me this sounds like something that could be achieved far more elegantly and efficiently using a HashMap that links up to an Operation that contains a Method and the number of parameters so I could each method to a HashMap like:

nameToOperation.put("+", new Operation("+", 1, Driver.class.getMethod("add")));
nameToOperation.put("car", new Operation("car", 1, Driver.class.getMethod("car")));

So there would be N different instances of the Operation class each containing the String, Method and number of parameters

And then I could simply call the method using something similar to this (I understand this isn't how you use invoke):

Operation op = ((Operation) expr.getData())

if(op.getNumPars() == 1)
    return(op.getMethod().invoke(expr.getNext()));
else
    return(op.getMethod().invoke(expr.getNext(), expr.getNext().getNext()));

However, I still don't fully like this solution as I am losing type safety and it still doesn't look that great. Another example I have seen on stackoverflow that looked quite elegant but I don't fully understand is the first solution of the top answer on: How to call a method stored in a HashMap? (Java)

What does everyone on Stackoverflow think the best solution is?

Edit: Just in case anybody searches this and was wondering about my solution, I made each operation such as Add, Car, Cdr have their own class that implemented Command. I then had to make the majority of my methods static, which I suppose by nature each of them were anyway. This seems way more elegant than the original case statement.

Community
  • 1
  • 1
  • This seems to be a use-case for the Action or [Commannd Pattern](http://en.wikipedia.org/wiki/Command_pattern). I've used this pattern in a [plugin application](https://github.com/RovoMe/PluginApplication/blob/master/PluginApplication/src/main/java/at/rovo/console/Console.java) to enable OSGi similar commands to load or unload plugins via console. The commands are added after initialization to the Console or could be even added via a further dynamically loaded plugin – Roman Vottner Dec 30 '13 at 16:53
  • I'd say that if you're only doing this in one place then stick with the switch statement. Using reflection or command pattern will just result in more lines of code and imo will not make the code more manageable. However if you're copy-pasting this switch statement (or parts of this switch statement) in multiple places then that is different and I would go with the command pattern in that case. – SamYonnou Dec 30 '13 at 17:00

2 Answers2

1

basicaly , the answer recommends to go with Command pattern.

"The main advantage of the command design pattern is that it decouples the object that invokes the operation from the one that know how to perform it. And this advantage must be kept. There are implementations of this design pattern in which the invoker is aware of the concrete commands classes. This is wrong making the implementation more tightly coupled. The invoker should be aware only about the abstract command class"
  1. Basicaly your map would be type safety. by declaring Map <character,Command>
  2. Open to Extendibility
Mani
  • 3,274
  • 2
  • 17
  • 27
  • Okay, this solution looks quite good. It seems I have to make a new class for each method so there is a AddMethod class that implements Command and the Map entry would be map.put("+", new AddMethod()); However, my add method made calls to other methods in the class and these now don't work due it it being in a different class, do you know how I could fix this? Edit: The switch statement was in a method called evaluate, and each method called from there has a call back to evaluate. – Kieran Maguire Dec 30 '13 at 17:30
  • yes, you need to have Seperate Class for each Command. i would also recommend to consider @Sam Yonnou comment, if you are planning to use it this in multiple place .. then go head with Command Pattern.Simple Example can be found http://en.wikipedia.org/wiki/Command_pattern – Mani Dec 30 '13 at 17:34
0

It looks like you are trying to write a Scheme interpreter. In that case you're gonna need a map anyway since you need to store all the user defined values und functions. When the user writes e.g. (define (add a b) (+ a b)), you store the function in the map using "add" as key. But your functions should use lists as inputs, i.e. each function has exactly one argument which is a list. In Scheme all expressions are lists by the way. Usually a Scheme interpreter consists of a reader and an evaluator. The reader converts the code into a bunch of nested lists. So basically "(define (add a b) (+ a b))" could be converted into a list structure similar to this.

List<Object> list = new ArrayList<Object>();
List<Object> list2 = new ArrayList<Object>();
list2.add("add"); list2.add("a"); list2.add("b");
List<Object> list3 = new ArrayList<Object>();
list3.add("+"); list3.add("a"); list3.add("b");
list.add("define"); list.add(list1); list.add(list2);

Of course your code doesn't actually look like this, instead the lists are constructed by recursive methods parsing the input code.

Those lists don't just contain strings btw., they also contain numbers and boolean values. Nested lists like this are the most simple form of an abstract syntax tree (AST). Since the syntax of Scheme is much simpler than that of most other languages, a very simple list structure is enough to store the parsed code.

The evaluator then processes those lists. To evaluate a list you first recursively evaluate every element in the list and then apply the first element to the rest of the list. That first element must therefore be a user defined function or a build in command e.g. "define".

Tesseract
  • 8,049
  • 2
  • 20
  • 37
  • Correct, I am already way into the project though probably too far to be changing my internal data structures. The way I am representing the data at the moment is using something similar to the dotted pairs notation where I have a hierarchy that: SExpression + Atom extend Expression So an SExpression has a data field and a next field where data is an Expression (Atom or another SExpression) and the next field is an SExpression. (I also had to put a parent field in). So when I call a procedure I send the next field. I handle user definitions by keeping them in an ArrayList of Definition objects – Kieran Maguire Dec 30 '13 at 18:04