6
\$\begingroup\$

Sometimes I need to write a simple Java console applications and menu look:

do {
    System.out.println("please select menu item");
    System.out.println("1-create, 2-remove, 3 - rename");
    int select;
    Scanner scanner = new Scanner(System.in);
    select = scanner.nextInt();

    switch (select) {
        case 1:
            System.out.println("please select menu item");
            System.out.println("1-image, 2-txt");
            select = scanner.nextInt();
            if (select == 1) fileHolder.create("image");
            else if (select == 2) fileHolder.create("image");
            else System.out.println("invalid menu item");
            break;
        case 2:
            System.out.println("please select menu item");
            System.out.println("1-file from pc1, 2-file from pc2, 3-file from pc3");
            select = scanner.nextInt();
            if (select == 1) fileHolder.removeFile("pc1");
            else if (select == 2) fileHolder.removeFile("pc2");
            else if (select == 3) fileHolder.removeFile("pc3");
            else System.out.println("invalid menu item");
            break;
        case 3:
            System.out.println("please type new name");
            String newName = scanner.next();
            fileHolder.rename(newName);
            break;
    }

} while (select != 4);

But I believe that it is not the best choice. I believe that I should use any pattern to improve my code. By description - the state pattern is applicable for my situation but I have no idea how I can use this in this situation. The menu should show the respective menu according to the internal state.

\$\endgroup\$
3
\$\begingroup\$

First of all, there are at least three buggy things:

  • select variable is declared inside the do... while loop, so the condition in while is checked against another one (probably, a field in the class?). So, the loop won't work as expected!

  • copy-paste is evil. The calls fileHolder.create("image"); are repeated in two different conditionals. The "txt" option is not used. Please pay more attention to the things you write and do not copy-paste such elementary things.

  • the Scanner is not closed after its usage, which is desirable, as for any other items that use I/O.

For the rest, the code seems quite straightforward, rigid and repetitive, hence difficult to maintain. It contains many if - else if clauses that smell bad.

Of course, the whole story may be refactored with patterns usage. But I don't think that the state pattern suits well here. It'd be better refactored with the command pattern.

All the menu actions (create, remove, rename and their dependent ones) may be wrapped into separate objects, which associate the expected keys (1,2,3..), the text and other necessary stuff and sugar.

For example, a (very short) version of Menu class could be like this:

class Menu {

  private final String action;
  private final Consumer<String> consumer;

  Menu(String action, Consumer<String> consumer) {
    this.action = action;
    this.consumer = consumer;
  }

  void execute() {
    this.consumer.accept(action);
  }
}

And it would be initialized and used:

Menu create = new Menu("create", (a) -> fileHolder.create(a));
// to trigger the execution:
create.execute();

But for this very concrete and short example of code I would consider decomposing it into patterns as a sort of overkill.

There is some job to do about the original straightforward approach, so let's just try to improve it.

The input numbers are read several times using the Scanner object. They should also be validated, so a dedicated method would be very useful:

// int minValue may also be defined if necessary
private static int askUserForNumberInput(Scanner scanner, String prompt, int maxValue) {
  System.out.println("please select menu item");
  System.out.println(prompt);
  int value = scanner.nextInt();
  while (value < 1 || value > maxValue) {
    System.out.println("invalid menu item, please try again");
    // java.util.InputMismatchException should also be caught 
    // to intercept non-numeric input
    value = scanner.nextInt();
  }
  return value;
}

The frame for the main logic would look like this:

try (Scanner scanner = new Scanner(System.in)) {
  final int mainMenuSelection = askUserForNumberInput(scanner, "1-create, 2-remove, 3 - rename", 3);
  switch (mainMenuSelection) {
    // cases
  }
}

And there is not a single if - if else in the cases any more:

case 1: {
  String[] createActions = { "image", "txt" }; // to be extracted to constants
  int createOption = askUserForNumberInput(scanner, "1-image, 2-txt", 2);
  fileHolder.create(createActions[createOption - 1]);
  break;
}
case 2: {
  int removeIndex = askUserForNumberInput(scanner, "1-file from pc1, 2-file from pc2, 3-file from pc3", 3);
  fileHolder.removeFile("pc" + removeIndex);
  break;
}
case 3: {
  System.out.println("please type new name");
  String newName = scanner.next();
  fileHolder.rename(newName);
  break;
}
| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

There is a lot of repetition in your code which is always a sure sign that some methods or classes should be introduced. It seems that you did understand that already.

I like your idea of using the state pattern. I made an implementation below where the state (the active menu) is kept in the variable menu of App. There can be a tree of sub-menus.

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

public class Menu {
    private final String name;
    private final String text;
    private LinkedHashMap<String, Runnable> actionsMap = new LinkedHashMap<>();

    public Menu(String name, String text) {
        this.name = name;
        this.text = text;
    }

    public void putAction(String name, Runnable action) {
        actionsMap.put(name, action);
    }

    public String generateText() {
        StringBuilder sb = new StringBuilder();
        sb.append(name).append(": ");
        sb.append(text).append(":\n");
        List<String> actionNames = new ArrayList<>(actionsMap.keySet());
        for (int i = 0; i < actionNames.size(); i++) {
            sb.append(String.format(" %d: %s%n", i + 1, actionNames.get(i)));
        }
        return sb.toString();
    }

    public void executeAction(int actionNumber) {
        int effectiveActionNumber = actionNumber - 1;
        if (effectiveActionNumber < 0 || effectiveActionNumber >= actionsMap.size()) {
            System.out.println("Ignoring menu choice: " + actionNumber);
        } else {
            List<Runnable> actions = new ArrayList<>(actionsMap.values());
            actions.get(effectiveActionNumber).run();
        }
    }

    public static class App {
        private Menu menu;
        private String password = "blahblah";

        public App() {
            Menu mainMenu = new Menu("Main", "main menu");
            Menu subMenuGetPassword = new Menu("Password", "get passwords");
            Menu subMenuSetPassword = new Menu("Set Password", "set password");

            mainMenu.putAction("get password menu", () -> activateMenu(subMenuGetPassword));
            mainMenu.putAction("quit", () -> System.exit(0));

            subMenuGetPassword.putAction("get password", () -> System.out.println(password));
            subMenuGetPassword.putAction("set password menu", () -> activateMenu(subMenuSetPassword));
            subMenuGetPassword.putAction("main menu", () -> activateMenu(mainMenu));
            subMenuGetPassword.putAction("quit", () -> System.exit(0));

            subMenuSetPassword.putAction("get password", this::setPassword);
            subMenuSetPassword.putAction("step back menu", () -> activateMenu(subMenuGetPassword));
            subMenuSetPassword.putAction("main menu", () -> activateMenu(mainMenu));
            subMenuSetPassword.putAction("quit", () -> System.exit(0));

            activateMenu(mainMenu);            
        }

        private void activateMenu(Menu newMenu) {
            menu = newMenu;
            System.out.println(newMenu.generateText());
            Scanner scanner = new Scanner(System.in);
            while (true) {
                // TODO some error checking.
                int actionNumber = scanner.nextInt();
                menu.executeAction(actionNumber);
            }
        }

        private void setPassword() {
            // TODO ask for password on command lin eand set it
            password = "p2";
        }
    }

    public static void main(String[] args) {
        new App();
    }
}
| improve this answer | |
\$\endgroup\$

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.