9 Replies Latest reply: Jun 13, 2011 3:27 AM by Opal RSS

    Too many if else's

    868017
      Hi
      I have a code which has too many if else conditions. Here is a snippet which I think is ugly.

      if (input.containsKey(rule)) {
      result.append(templateInputs.get(rule));
      } else if (rule.equals("#form(date,\"d\")#")) {
      result.append(someObj.getDate().get(Calendar.DATE));
      } else if (rule.equals("#form(date,\"m\")#")) {
      result.append(someObj.getDate().get(Calendar.MONTH));
      } else
      if (rule.equals("#form(date,\"m/d/yyyy\")#")) {
      result.append(DateUtils.formatDate(someObj.getDate().getTime()));
      } else
      if (rule.equals("#form(date,\"mmmm d\")#")) {
      result.append(MONTHS[someObj.getDate().get(Calendar.MONTH)]);
      result.append(someObj.getDate().get(Calendar.DATE));
      } else ......

      The list is pretty long. Now I dont see a method to rewrite this polymorphically or in a rules pattern. Does anyone have any idea about how to better this ?
      Thanks in advance.
        • 1. Re: Too many if else's
          796440
          If the initial input.containsKey(rule) test is the only oddball and the rest are all rule.equals(), then for all but that initial special case, you could use the Command Pattern. Googling for java command pattern should turn up lots or explanations and examples.

          (Note that the Command Pattern, and Design Patterns in general, are not specific to Java. They're language-agnostic for the most part. Including "java" in your search, however, will get you examples of how to implement it in Java.)

          There are of course other approaches, but without any real requirements or context, it would be pointless to speculate on what else might be appropriate.
          • 2. Re: Too many if else's
            EJP
            It looks to me like you need an abstract Rule object with an execute() method that takes someObj and result as parameters; then you instantiate an anonymous instance per rule with its own execute() method.
            • 3. Re: Too many if else's
              jschellSomeoneStoleMyAlias
              The list is pretty long
              Which means 10, 100 or 1000?
              • 4. Re: Too many if else's
                868017
                Around 20
                • 5. Re: Too many if else's
                  868017
                  Is this what you are suggesting

                  class Rule1 implements Command {
                  public void execute(String rule, StringBuffer result ) {
                  if(rule.equals("#form(date,\"mmmm d\")#") {
                            result.append(MONTHS[someObj.getDate().get(Calendar.MONTH)]);     
                            }
                  }
                  }

                  class Rule2 implements Command {
                  public void execute(String rule, StringBuffer result ) {
                  if (rule.equals("#form(date,\"m/d/yyyy\")#")) {
                            result.append(DateUtils.formatDate(someObj.getDate().getTime()));
                       }
                  }
                  }
                  • 6. Re: Too many if else's
                    EJP
                    More like this:
                    Map<String, Command> commands = new HashMap<String, Command>();
                    // Populate the map
                    commands.put("#form(date,\"mmmm d\")#", new Command(){
                      public void execute(SomeObject someObj, StringBuffer result)
                      {
                        result.append(MONTHS[someObj.getDate().get(Calendar.MONTH)]); 
                      }
                    });
                    // etc for all other rules
                    
                    // Then to execute a rule:
                    Command cmd = commands.get(rule); 
                    if (cmd != null)
                        command.execute(someObj, result);
                    • 7. Re: Too many if else's
                      Opal
                      The code is redundant - many very similar classes. Look at this:
                      enum Rules {
                      
                           RULE1("rule1"), RULE2("rule2");
                      
                           private String rule;
                      
                           private Rules(String rule) {
                                this.rule = rule;
                           }
                      
                           public static Rules getEnumForRule(String rule) {
                                for (Rules r : values()) {
                                     if (rule.equals(r.rule)) {
                                          return r;
                                     }
                                }
                                return null;
                           }
                      }
                      
                      class RuleFormatter {
                           public static String format(Rules r) {
                                switch (r) {
                                case RULE1:
                                     return "";
                                case RULE2:
                                     return "";
                                default:
                                     return "";
                                }
                           }
                      }
                      • 8. Re: Too many if else's
                        EJP
                        Your RuleFormatter class misses the point completely. The format method should obviously be in the Enum, with specializations for every element. In which case it becomes just another notation of the Command pattern, which is what everybody else has been talking about.

                        Speaking of redundancy, your getEnumForRule() is redundant given that the compiler already generates a Rules.valueOf(String rule) method.

                        In the face of this misfeature:
                        Enum Command
                        {
                          private String rule;
                          public Command(String rule)
                          {
                            this.rule = rule;
                          }
                        
                          public abstract void execute(SomeObject someObj, StringBuffer result);
                        
                          RULE1("#form(date,\"mmmm d\")#")
                          {
                            public void execute(SomeObject someObj, StringBuffer result)
                            {
                              result.append(MONTHS[someObj.getDate().get(Calendar.MONTH)]); 
                            }
                          },
                          // etc for all other rules
                          
                        } 
                        
                        // Then to execute a rule:
                        Command cmd = Command.valueOf(rule);  // catch NoSuchElementException
                        cmd.execute(someObj, result);
                        • 9. Re: Too many if else's
                          Opal
                          Agreed. Thanks for the hints.