8
\$\begingroup\$

I am a Spanish programming teacher in a vocational training course and I want my students to make a CRUD exercise on arrays. I want them to start using OOP as well and as an example, I'm going to give them this example of OO Console Menu. We haven't seen Java 8 so far. Any comments or improvements would be very appreciate.

MenuOption.java

package es.primerok.menu;

/**
* Created by Miguel-David Orrico on 18/02/2017.
*/
public abstract class MenuOption {
private String option;
private String menuLine;

public MenuOption(String option, String menuLine) {
    this.option = option;
    this.menuLine = menuLine;
}

@Override
public String toString() {
    return this.option+" - "+this.menuLine;
}

public String getOption() {
    return option;
}

public abstract void doAction();

}

Menu.java

package es.primerok.menu;

import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;

/**
 * Created by Miguel-David Orrico on 18/02/2017.
 */
public class Menu {
private static final String OPTION_EXIT="0";
private static final String LEVEL_INDENTATION ="    ";
private List<MenuOption> menuOptions=new ArrayList<>();
private int level=0;

public Menu(){}

public Menu(int level){
    this.level=level;
}

public void add(MenuOption m){
    this.menuOptions.add(m);
}

public void loopUntilExit(){

    String input;
    try(Scanner sc=new Scanner(System.in)) {
        while (true) {
            System.out.println(this);
            input = sc.nextLine();
            if (input.equals(OPTION_EXIT)) {
                return;
            }
            boolean foundOption = false;
            for (MenuOption m : menuOptions) {
                if (m.getOption().equalsIgnoreCase(input)) {
                    m.doAction();
                    foundOption = true;
                }
            }
            if (!foundOption) {
                System.out.println("Sorry, unrecognized option");
            }
        }
    }
}

@Override
public String toString(){
    StringBuilder sb = new StringBuilder();
    sb.append("Choose an option, "+OPTION_EXIT+" to exit.\n");
    for(MenuOption m:menuOptions){
        for(int i=0;i<level;i++){
            sb.append(LEVEL_INDENTATION);
        }
        sb.append(m).append("\n");
    }
    return sb.toString();
}
}

MenuTest.java

package es.primerok.menu;

/**
 * Created by Miguel-David Orrico on 18/02/2017.
 */
public class MenuTest {
public static void main(String[] args){

    System.out.println("es.primerok.menu.Menu Test");
    Menu mainMenu=new Menu();
    Menu subMenu=new Menu(1);

    subMenu.add(new MenuOption("+","Add numbers") {
        @Override
        public void doAction() {
            System.out.println("5+7=12");
        }
    });

    subMenu.add(new MenuOption("-","Subtract some numbers") {
        @Override
        public void doAction() {

            System.out.println("we're subtracting 6 and 5. The result is 1");
        }
    });
    mainMenu.add(new MenuOption("1","Option number 1") {
        @Override
        public void doAction() {
            System.out.println("This is number one");
        }
    });
    mainMenu.add(new MenuOption("A","Print an A") {
        @Override
        public void doAction() {
            System.out.println("I'm in Option A");
        }
    });
    mainMenu.add(new MenuOption("sub","This has a submenu") {
        @Override
        public void doAction() {
            subMenu.loopUntilExit();
        }
    });

    mainMenu.loopUntilExit();
}
}
\$\endgroup\$
  • \$\begingroup\$ Menu is a class already provided by the JVM. Do not hide JVM classes by Naming your own the same! \$\endgroup\$ – Timothy Truckle Mar 5 '17 at 17:37
6
\$\begingroup\$

Consistent style

As a teacher, try to lead by example. Sometimes you put whitespace around operators (which is the norm), sometimes you don't, sometimes you prefix an opening brace with space, sometimes you don't. Whatever you do, make sure it is consistent (or configure a code format in your IDE and run the formatter over the whole code.)

Non-OO menu decision

In OO you don't just rip information out of an object to base your decision on it. Instead of

 if (m.getOption().equalsIgnoreCase(input))

add a method to the MenuOption, to let the option decide, whether it is suited for the input

 if(m.matchesInput(input))

Variable scope

In your loopUntilExit(), you declare the input String outside the loop - why? This should go to the minimal acceptable scope, that is in the line where you call sc.nextLine().

Law of least surprises

I don't like abusing the toString() method to create a multi-line screen menu. This is not what I'd expect of toString(). Thus, I suggest you create another method to do this, and call it explicitly.

One afterthought

In 20 years of programming Java, I have never seen the use of Scanner in real life (TM). Is it really necessary, that every teacher seems to love this object and base their classes on it? Yes, it is simple, straightforward to use, and probably an easy-to-explore way to teach other principles without the input getting in your way, but it does not hold any practical relevance (at least as far I am concerned.)

\$\endgroup\$
  • \$\begingroup\$ "I have never seen the use of Scanner in real life (TM)." The scanner class may have its relevance when you process CSV files, where you have a well defined column structure (as long a no string values occur...) \$\endgroup\$ – Timothy Truckle Mar 5 '17 at 17:42
4
\$\begingroup\$

1) Use Test Driven development. MenuTest is not a proper testcase, it is an application. A testcase must include Pre-Condition(s), Execution and Post-Condition(s) to be valid and valuable. c.f. Hoare Logic. You have an execution but you have no way to tell if it has actually met the requirements, or even if the pre-requisites are true. Instilling Test Driven Development is probably the single most important idea you can teach beginning programmers.

2) There is no proper separation of concerns, apply the single responsibility principle, one class does one thing. Firstly separate behaviour from presentation.

3) You are passing properties to your Menu items, however they all the menu items exhibit the same actual behaviour. Use the Command Pattern to provide different behaviour polymorphically; you have half of it done anyway. Each command would make an excellent way to learn/practice TDD.

4) Follow the Tell don't Ask idiom, properly called the Law of Demeter. Avoid 'getters', they increase coupling between caller and callee. They really should considered an evil anti-pattern for beginners and should only be used when the dev understands and accepts the couple consequences.

5) Name things for the Problem domain language not using programming jargon. This will also help with item 2) above.

\$\endgroup\$
  • \$\begingroup\$ "Use Test Driven development. [...] A testcase must include [...]" and most important with TDD: any test method within a test class verifies a single expectation about the behavior of the production code. \$\endgroup\$ – Timothy Truckle Mar 5 '17 at 17:34

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.