11
\$\begingroup\$

In this program, I have to check the range of a given age and then print a message as per the age. I want to optimize the if else logic.

/* Sample code to read in test cases:*/

import java.io.*;
public class Main {
 public static void main (String[] args) throws IOException {
    File file = new File(args[0]);
    BufferedReader buffer = new BufferedReader(new FileReader(file));
    String line;
    while ((line = buffer.readLine()) != null) {
        line = line.trim();
        // Process line of input Here

        int age = Integer.parseInt(line);

          if(age>=0 && age<=2){
              System.out.println("Still in Mama's arms");
             // break;
          }else if(age>=3 && age<=4){
               System.out.println("Preschool Maniac");
              // break;
          }else if(age>=5 && age<=11){
               System.out.println("Elementary school");
             // break;
          }else if(age>=12 && age<=14){
               System.out.println("Middle school");
             // break;
          }else if(age>=15 && age<=18){
               System.out.println("High school");
             // break;
          }else if(age>=19 && age<=22){
               System.out.println("College");
             // break;
          }else if(age>=23 && age<=65){
               System.out.println("Working for the man");
             // break;
          }else if(age>=66 && age<=100){
               System.out.println("The Golden Years");
              //break;
          }else{
               System.out.println("This program is for humans");

          }
         }
      }
    }

Is there any other way to do this program? Using a switch, it becomes more complex to type with so many cases for the age.

\$\endgroup\$
  • 27
    \$\begingroup\$ I'm somewhat amused by the fact that people over 100 years old aren't human. \$\endgroup\$ – cHao Aug 9 '16 at 8:25
  • 2
    \$\begingroup\$ You can leave out all but the first (age >= ... parts - they're covered by the previous checks already. \$\endgroup\$ – Pieter Witvoet Aug 9 '16 at 8:43
  • 1
    \$\begingroup\$ @PieterWitvoet Wrong. Try thinking what happens for "-2" as input. It needs to pass through all cases to hit the bottom one. \$\endgroup\$ – Pimgd Aug 9 '16 at 8:44
  • 1
    \$\begingroup\$ @Pimgd: good call, that requires an extra check up-front. \$\endgroup\$ – Pieter Witvoet Aug 9 '16 at 8:47
  • \$\begingroup\$ IMO that code is fine as is. It's extremely easy to read... you may dislike it as the writer, but the next person who reads that code will be perfectly happy. \$\endgroup\$ – gardenhead Aug 9 '16 at 16:16
20
\$\begingroup\$

A TreeMap<Integer, String> with a for loop and stop on first number greater than or equaling the number to find will solve your problem.

treemap.put(-1, "This program is for humans");
treemap.put(2, "Still in Mama's arms");
//etc...
treemap.put(Integer.MAX_VALUE, "This program is for humans");

Then, loop through entries...

for(Map.Entry<Integer, String> entry : treemap.entrySet()){
    if(entry.getKey() >= age){
        System.out.println(entry.getValue());
        break;
    }
}

The default behaviour for a TreeMap is that the contents are sorted based on the key, which explains why this works. Use Integer.MAX_VALUE for the last case - it'll be equal to or greater than any other number.

You can even use a built-in function for this:

System.out.println(treemap.ceilingEntry(age).getValue());

Making the final code...

import java.io.*;
import java.util.TreeMap;

public class Main {
    private static TreeMap<Integer, String> treemap = new TreeMap<>();

    private static void initializeAgeDescriptions() {
        treemap.put(-1, "This program is for humans");
        treemap.put(2, "Still in Mama's arms");
        treemap.put(4, "Preschool Maniac");
        treemap.put(11, "Elementary school");
        treemap.put(14, "Middle school");
        treemap.put(18, "High school");
        treemap.put(22, "College");
        treemap.put(65, "Working for the man");
        treemap.put(100, "The Golden Years");
        treemap.put(Integer.MAX_VALUE, "This program is for humans");
    }

    public static void main (String[] args) throws IOException {
        initializeAgeDescriptions();
        File file = new File(args[0]);
        BufferedReader buffer = new BufferedReader(new FileReader(file));
        String line;
        while ((line = buffer.readLine()) != null) {
            line = line.trim();

            int age = Integer.parseInt(line);
            System.out.println(treemap.ceilingEntry(age).getValue());
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ thanks for nice solution. I have an query here, Don't you think storing the value in treeMap then retrieving them back adding some extra processing time. \$\endgroup\$ – Prabhat Yadav Aug 9 '16 at 9:03
  • \$\begingroup\$ @PrabhatYadav It does add some time, but I think this is in the range of less than 0.001 seconds. \$\endgroup\$ – Pimgd Aug 9 '16 at 9:04
  • 9
    \$\begingroup\$ The tiny performance penalty would be so much more preferable to the developer performance penalty your original if statements had \$\endgroup\$ – Dan Pantry Aug 9 '16 at 9:07
  • 9
    \$\begingroup\$ Alternatively, you can use treemap.ceilingEntry(age).getValue() without the for loop. \$\endgroup\$ – Tunaki Aug 9 '16 at 10:23
  • 2
    \$\begingroup\$ @Y.B. Just... no. Now you're being pedantic. Yes there will be unboxing; how performant does this need to be? In my opinion, the code I posted is fast enough (less than a couple milliseconds). If you think this is really bad advice I am giving, then that's fine; I prefer readability and extensibility over fast and unmaintainable. As for the import, I assume this will be added by the programmer; I personally don't care about imports myself and let my IDE handle such things. \$\endgroup\$ – Pimgd Aug 9 '16 at 18:38
10
\$\begingroup\$

Ternary operator can be used to choose parameter for just one System.out.println call:

System.out.println(
    age <   0 ? "This program is for the living" :
    age <   3 ? "Still in Mama's arms" :
    age <   5 ? "Preschool Maniac" :
    age <  12 ? "Elementary school" :
    age <  15 ? "Middle school" :
    age <  19 ? "High school" :
    age <  23 ? "College" :
    age <  66 ? "Working for the man" :
    age < 101 ? "The Golden Years" :
    age < 201 ? "Revered Ancient" :
                "This program is for mere mortals"
);

Update following the comments:

  1. Ternary operator is not exactly the same as if / else. It can be used on the right-hand side of Java statements. Here it is used to define which parameter to pass to a function, rather than which function with pre-defined parameter to call (note one System.out.println);
  2. Conditions are re-written to avoid comparison duplication (no &&);
  3. Bearing in mind age is int (thanks, @Andrew Coonce) we can use < instead of <=;
  4. Compact, easy to read and maintain code (no brackets, else if etc.).

Update: OP asked for if / else logic optimization only, but for the posterity I would like to note that BufferReader must be closed. Ideally - automatically in try-with-resources block. Since main (String[] args) throws IOException we would have to re-throw it.

There is also a great example of how to read lines from file through for cycle rather than while.

We also might want to handle NumberFormatException in Integer.parseInt and the result would be:

import java.io.*;

public class Main {
  public static void main (String[] args) throws IOException {
    try (BufferedReader buffer = new BufferedReader(new FileReader(new File(args[0])))) {
      for (String line = buffer.readLine(); line != null; line = buffer.readLine()) {
        try {
          int age = Integer.parseInt(line.trim());

          System.out.println(
            age <   0 ? "This program is for the living" :
            age <   3 ? "Still in Mama's arms" :
            age <   5 ? "Preschool Maniac" :
            age <  12 ? "Elementary school" :
            age <  15 ? "Middle school" :
            age <  19 ? "High school" :
            age <  23 ? "College" :
            age <  66 ? "Working for the man" :
            age < 101 ? "The Golden Years" :
            age < 201 ? "Revered Ancient" :
                        "This program is for mere mortals"
          );
        } catch (NumberFormatException e) {
          System.out.println("Fortunate One of Unguessable Age");
        }
      }
    } catch (IOException e) {
      throw e;
    }
  }
}

Technically, in main we should also handle ArrayIndexOutOfBoundsException when no parameters are passed, but I think in real code it won't be main, but rather a procedure that can not be called with parameter missing.

\$\endgroup\$
  • 1
    \$\begingroup\$ this is not different..... OP says: "Refactoring many if / else if statements" \$\endgroup\$ – jeremy Aug 9 '16 at 12:29
  • 5
    \$\begingroup\$ @jeremy: I would argue that modifying the syntactic sugar to make it more readable to the average developer satisfies both the letter and spirit of the request. I much prefer this solution, especially if we were to need to modify the request to use "<" instead of "<=". \$\endgroup\$ – Andrew Coonce Aug 9 '16 at 13:49
  • 12
    \$\begingroup\$ Nested ternaries are the devil's work. \$\endgroup\$ – MikeTheLiar Aug 9 '16 at 14:13
  • \$\begingroup\$ @mikeTheLiar agreed, but as much as I dislike them, this is surprisingly readable. \$\endgroup\$ – Jules Aug 9 '16 at 19:33
2
\$\begingroup\$

I prefer use simple function, something like:

/**
 * If minAge <= age < maxAge is satisfied, prints text and returns true.
 * Else prints nothing and returns false.
 */
private boolean testAndPrintIf(int age, int minAge, int maxAge, String text) {
    if (age >= minAge && age < maxAge) {
        System.out.println(text);
        return true;
    } else {
        return false;
    }
}

So, you specify the "dictionary" in the code, like:

testAndPrintIf(age, 0, 2, "Still in Mama's arms");
testAndPrintIf(age, 2, 4, "Preschool Maniac");
testAndPrintIf(age, 5, 12, "Elementary school");
//...

Obviously, this solution does not better the linear time. One can eventually chain calls using the "or" operator (which causes to stop testing when first condition is satisfied):

testAndPrintIf(age, 0, 2, "Still in Mama's arms")
|| testAndPrintIf(age, 2, 4, "Preschool Maniac")
|| testAndPrintIf(age, 5, 12, "Elementary school")
//...

The time will be optimal only if you use some hash table with very specific and complex hash function(s), but - in this case it is not necessary(!).

\$\endgroup\$
  • 1
    \$\begingroup\$ The else could be removed, just return false via implicit else... and the maxage is inclusive in OP's example \$\endgroup\$ – Pimgd Aug 9 '16 at 12:12
-2
\$\begingroup\$

With Pimgd's great and easy to use answer, I will provide a bit more of an (over engineerd, if you will) OOP solution that combines his answer and my own thoughts.

Consider a class called AgeDescription:

public class AgeDescription {
    private int age;
    private String description;

    public AgeDescription(int age, String descrption) {
        this.age = age;
        this.description = description;
    }

    public int getAge() [
        return age;
    }

    public String getDescription() {
        return description;
    }
}

Since there is a bunch of logic behind determining what description belongs to what age, this logic needs to be encapsulated, reusable and testable. For this we can use the Factory design-pattern:

public class AgeDescriptionFactory {
    private static TreeMap<Integer, String> ageDescriptionTreeMap = new TreeMap<>();

    static {
        // This gets executed once to fill up the TreeMap.
        ageDescriptionTreeMap.put(2, "Still in Mama's arms");
        ageDescriptionTreeMap.put(4, "Preschool Maniac");
        // etc...
        ageDescriptionTreeMap.put(Integer.MAX_VALUE, "This program is for humans");
    }


    public AgeDescription createFromAge(int age) {
        if (age < 0) {
            throw new InvalidArgumentException("Age cannot be negative");
        }

        return new AgeDescription(age, getDescriptionFromAge(age));
    }

    private String getDescriptionFromAge(int age) {
        return ageDescriptionMap.ceilingEntry(age).getValue();
    }
}

Thank you @Tunaki for mentioning the ceilingEntry function in your comment

I think checking for a negative age is a good thing to do, because it is technically not a valid argument. Really large positive numbers could be actual ages in the near future when medicine and what not have progressed tremendously, but negative ages I don't ever see existing.

Now you can use the classes like this:

// Retrieve age variable from user input    

AgeDescriptionFactory ageDescriptionFactory = new AgeDescriptionFactory();
AgeDescription ageDescription = ageDescriptionFactory.createFromAge(age);

System.out.println(ageDescription.getDescription());

As an additional benefit, the code is now easily testable:

public class AgeDescriptionFactoryTest {
    private AgeDescriptionFactory ageDescriptionFactory;

    // This method is run before each test
    @before
    public void setUp() {
        this.ageDescriptionFactory = new AgeDescriptionFactory();
    }

    @Test(expected=InvalidArgumentException.class)
    public void testNegativeAgeThrowsException {
        this.ageDescriptionFactory.createFromAge(-1);
    }

    @Test
    public void testAgeInFourtiesReturnsWorkingMan {
        AgeDescription ageDescription = ageDescriptionFactory.createFromAge(44);
        Assert.equals("Working for the man", ageDescription.getDescription();
    }

    // Keep on testing!
}

A good thing to take away from this is that you want to keep your logic seperate from what is done with that logic. You have intertwined outputting the description of an age with the actual logical proces of determining this description based on the age. There is no way to use that logic for anything else. By seperating this, you get code that is testable and reusable.

\$\endgroup\$
  • \$\begingroup\$ This... doesn't solve the massive if-else-if chain. I don't see how this improves the code. It's only once you add the map in that this becomes a bit better... but only by stowing the map creation into a single spot. \$\endgroup\$ – Pimgd Aug 9 '16 at 9:23
  • \$\begingroup\$ That's not versatile at all, you keep the if chaining. The rest is a good idea though. \$\endgroup\$ – Mast Aug 9 '16 at 9:27
  • \$\begingroup\$ I also noted that you can use your answer to remove the if chaining but keep the benefits of the abstraction \$\endgroup\$ – Thijs Riezebeek Aug 9 '16 at 9:28
  • \$\begingroup\$ I think if you take the map at the start and build from there your answer is a lot stronger because it provides OP with a OOP solution of how to apply the map solution, rather than first advising the creation of a ton of ... storage classes, and then later combining everything together. Also, final on age description fields might be worth something...? \$\endgroup\$ – Pimgd Aug 9 '16 at 9:32
  • 1
    \$\begingroup\$ see updated answer; tunaki resolves reading from the map to a oneliner \$\endgroup\$ – Pimgd Aug 9 '16 at 12:14

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.