5
\$\begingroup\$

This video should explain how the game works, but in simple form, it's a game where you enter a number between 1 and 3, This number is than removed from 12, in doing so the computer will then pick a number between 1-3 based on what number you have picked, if you choose to remove 1 marble it should say that it has removed 3 marbles. This works because there are 3 lots of 4 in 12. And the maths tells it to remove what number you have chosen from 4. Because there are 3 lots of 4, you can never win, which is the whole point of the game.

Is there any way I could improve on the code?

 import java.util.Scanner;

    public class Main {

        public static int input;
        public static int marble = 12;
        public static int comFirst = (int) (Math.random() * 3 + 1);

        @SuppressWarnings("resource")
        public static void main(String[] args) throws InterruptedException {

            System.out.println("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Time");
            System.out.println("Your Goal Is To Remove The Last Marble, Marble 1");
            System.out.println("To Remove A Marble Either Enter 1, 2 or 3");
            System.out.println("");

            while (true) {

                if (marble == 1) {
                    System.out.println("There Is " + marble + " Marble Remaining");
                } else {
                    System.out.println("There Are " + marble + " Marbles Remaining");
                }

                Scanner scanInput = new Scanner(System.in);
                input = scanInput.nextInt();

                if (input > 3 || input < 1) {
                    System.out.println("You Can Only Remove 3 Marbles At A Time");
                } else {

                    marble = marble - input;

                    Thread.sleep(250);
                    int cpuAnswer = 4 - input;
                    if (cpuAnswer == 1) {
                        System.out.println("The Computer Has Removed " + cpuAnswer + " Marble");
                    } else {
                        System.out.println("The Computer Has Removed " + cpuAnswer + " Marbles");
                    }
                    if (marble - cpuAnswer == 0) {
                        System.out.println("There Are No Marbles Remaining, The Computer Has Won!!");
                        System.out.println("The Computer Has Won");
                        return;
                    }
                    if (marble - input == 0) {
                        System.out.println("There Are No Marbles Remaining, You Have Won!!");
                        return;
                    }
                    marble = marble - cpuAnswer;

                }
            }
        }

    }
\$\endgroup\$
9
  • 1
    \$\begingroup\$ BTW if your wondering why i added the player winning code is so that if someone checks the source code they will still think that they have a chance of winning. \$\endgroup\$
    – Dragon4c3_
    Feb 2, 2016 at 9:35
  • 1
    \$\begingroup\$ what's the goal of the game again? \$\endgroup\$
    – Vogel612
    Feb 2, 2016 at 10:20
  • \$\begingroup\$ To lose. Watch the video, it's a game from the 60's in which it's impossible to win, you can use it for gambling which i wouldn't recommend. \$\endgroup\$
    – Dragon4c3_
    Feb 2, 2016 at 10:21
  • 3
    \$\begingroup\$ I'm in no mood to watch random videos. aside from that I currently have no sound. As a result your question is incomplete for me. Other users may have no access to youtube because of corporate policies. Please explain the rules of the game clearly in your question to allow everybody to answer \$\endgroup\$
    – Vogel612
    Feb 2, 2016 at 10:25
  • 1
    \$\begingroup\$ @Vogel612 The goal of the game is to be the one who takes the last marble. \$\endgroup\$ Feb 2, 2016 at 10:44

3 Answers 3

6
\$\begingroup\$

Break early instead of nested if

           if (input > 3 || input < 1) {
               System.out.println("You Can Only Remove 3 Marbles At A Time");
           } else {
               ....
           }

The above can be rewritten as a simple if statement with continue, this prevent the indention from creeper up at the side of the screen.

           if (input > 3 || input < 1) {
               System.out.println("You Can Only Remove 3 Marbles At A Time");
               continue;
           }
           ....

Placing the scanner outside the loop

Scanners use internal buffering to buffer its inputstream, this mean they may consume large amounts, instead of only the part they need for the the concurrent operation. When placing the scanner outside the loop, this also allows for the player to put in the moves much quicker after each other, an still having the program pick up every move.

Place Scanner scanInput = new Scanner(System.in); before while(true){, and wrap the scanner in a try with resources like this:

try(Scanner scanInput = new Scanner(System.in)){
    while(true) { 
        ....
    }
}

This also allows you to get rid of @SuppressWarnings("resource").

Proper english doesn't have all words title case

(all println statements)

SOme people have problems with reading sentences with every word uppercase, use the english grammar rules, make only names and the first word of a sentence starting with a uppercase.

Check the state of scanInput.hasNextInt()

You should check the state of scanInput.hasNextInt() before every number is get, this method can return false if the program is accessed by the command line and the command line return EOF, this can be triggered with ctrl + D on linux. The best way to do this is replacing the true in your while loop with the check.

Unused variables

public static int comFirst = (int) (Math.random() * 3 + 1);

At the moment, you have 1 unused variable. Either you will be using this variable for a future expansion, or this variable is left while you were working on the application.

You have won case triggers after the computer move

This case triggers after the computer has done its move. If you really planning on the player to win sometime, you should move this case before the computer makes a move.

Win cases too strict

if (marble - cpuAnswer == 0) {
if (marble - input == 0) {

While with the current implementation of your application, all wins are perfect and ending with exactly 0. If you update your implementation of the computer or player in the future, this may pose a problem in the future.

if (marble - cpuAnswer =< 0) {
if (marble - input =< 0) {

The final result would look like this:

import java.util.Scanner;

public class Main {

    public static int input;
    public static int marble = 12;

    public static void main(String[] args) throws InterruptedException {

        System.out.println("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Time");
        System.out.println("Your Goal Is To Remove The Last Marble, Marble 1");
        System.out.println("To Remove A Marble Either Enter 1, 2 or 3");
        System.out.println("");

        try (Scanner scanInput = new Scanner(System.in)) {
            while (scanInput.hasNextInput()) {

                if (marble == 1) {
                    System.out.println("There Is " + marble + " Marble Remaining");
                } else {
                    System.out.println("There Are " + marble + " Marbles Remaining");
                }

                input = scanInput.nextInt();

                if (input > 3 || input < 1) {
                    System.out.println("You Can Only Remove 3 Marbles At A Time");
                    continue;
                } 
                marble = marble - input;
                if (marble - input =< 0) {
                    System.out.println("There Are No Marbles Remaining, You Have Won!!");
                    return;
                }
                Thread.sleep(250);
                int cpuAnswer = 4 - input;
                if (cpuAnswer == 1) {
                    System.out.println("The Computer Has Removed " + cpuAnswer + " Marble");
                } else {
                    System.out.println("The Computer Has Removed " + cpuAnswer + " Marbles");
                }
                if (marble - cpuAnswer =< 0) {
                    System.out.println("There Are No Marbles Remaining, The Computer Has Won!!");
                    System.out.println("The Computer Has Won");
                    return;
                }

                marble = marble - cpuAnswer;
            }
        }
    }
}

Splitting the main method

By splitting the main method you archieve better code reuse.

@TheCoffeeCup has written an excellent answer for this part of the code.

\$\endgroup\$
6
  • \$\begingroup\$ I know that English doesn't use all uppercase letters but for me it looks better, and this project is only going to be for my learning, i will use lowercase letters in projects i plan to release, i added try(Scanner scanInput = new Scanner(System.in)){ while(true) { .... } } and it made the console spam you can only use numbers between 1-3. \$\endgroup\$
    – Dragon4c3_
    Feb 2, 2016 at 10:12
  • \$\begingroup\$ And i don't understand Check the state of scanner.hasNextInt() what do i replace the while(true) loop with? \$\endgroup\$
    – Dragon4c3_
    Feb 2, 2016 at 10:16
  • \$\begingroup\$ @Dragon4c3_ I added code how the post would look like after applying most of the steps \$\endgroup\$
    – Ferrybig
    Feb 2, 2016 at 10:23
  • \$\begingroup\$ Don't put everything in main(). It's a bad idea. \$\endgroup\$ Feb 3, 2016 at 18:52
  • \$\begingroup\$ @TheCoffeeCup Can you add a answer for that? I am bad at explaining how to split up a main method. \$\endgroup\$
    – Ferrybig
    Feb 3, 2016 at 19:03
2
\$\begingroup\$

To continue from @FerryBig's answer:

His code looks like:

import java.util.Scanner;

public class Main {

    public static int input;
    public static int marble = 12;

    public static void main(String[] args) throws InterruptedException {

        System.out.println("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Time");
        System.out.println("Your Goal Is To Remove The Last Marble, Marble 1");
        System.out.println("To Remove A Marble Either Enter 1, 2 or 3");
        System.out.println("");

        try (Scanner scanInput = new Scanner(System.in)) {
            while (scanInput.hasNextInput()) {

                if (marble == 1) {
                    System.out.println("There Is " + marble + " Marble Remaining");
                } else {
                    System.out.println("There Are " + marble + " Marbles Remaining");
                }

                input = scanInput.nextInt();

                if (input > 3 || input < 1) {
                    System.out.println("You Can Only Remove 3 Marbles At A Time");
                    continue;
                } 
                marble = marble - input;
                if (marble - input =< 0) {
                    System.out.println("There Are No Marbles Remaining, You Have Won!!");
                    return;
                }
                Thread.sleep(250);
                int cpuAnswer = 4 - input;
                if (cpuAnswer == 1) {
                    System.out.println("The Computer Has Removed " + cpuAnswer + " Marble");
                } else {
                    System.out.println("The Computer Has Removed " + cpuAnswer + " Marbles");
                }
                if (marble - cpuAnswer =< 0) {
                    System.out.println("There Are No Marbles Remaining, The Computer Has Won!!");
                    System.out.println("The Computer Has Won");
                    return;
                }

                marble = marble - cpuAnswer;
            }
        }
    }
}

The issue with that code is that everything is in the main method. Another issue is that it doesn't follow any OOP principle.

But first, let's fix the code. A lot of it can be replaced with ternary operators.

1:

if (marble == 1) {
    System.out.println("There Is " + marble + " Marble Remaining");
} else {
    System.out.println("There Are " + marble + " Marbles Remaining");
}

Can be changed to:

System.out.println("There " + (marble == 1 ? "Is" : "Are")
        + " " + marble + " Marble" + (marble == 1 ? "" : "s")
        + " Remaining");

Ugly. Try printf:

System.out.println("There %s %d %s Remaining",
        (marble == 1 ? "Is" : "Are"),
        marble,
        (marble == 1 ? "Marble" : "Marbles"));

2:

if (cpuAnswer == 1) {
    System.out.println("The Computer Has Removed " + cpuAnswer + " Marble");
} else {
    System.out.println("The Computer Has Removed " + cpuAnswer + " Marbles");
}

To:

System.out.println("The Computer Has Removed " + cpuAnswer +
        (cpuAnswer == 1 ? " Marble" : " Marbles"));

3:

marble = marble - cpuAnswer;

To:

marble -= cpuAnswer;

4:

System.out.println("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Time");
System.out.println("Your Goal Is To Remove The Last Marble, Marble 1");
System.out.println("To Remove A Marble Either Enter 1, 2 or 3");
System.out.println("");

To:

    System.out.println("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Time\n"
            + "Your Goal Is To Remove The Last Marble, Marble 1\n"
            + "To Remove A Marble Either Enter 1, 2 or 3\n");

After:

import java.util.Scanner;

public class Main {

    public static int input;
    public static int marble = 12;

    public static void main(String[] args) throws InterruptedException {

        System.out.println("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Time\n"
                + "Your Goal Is To Remove The Last Marble, Marble 1\n"
                + "To Remove A Marble Either Enter 1, 2 or 3\n");

        try (Scanner scanInput = new Scanner(System.in)) {
            while (scanInput.hasNextInput()) {

                System.out.println("There %s %d %s Remaining",
                        (marble == 1 ? "Is" : "Are"),
                        marble,
                        (marble == 1 ? "Marble" : "Marbles"));

                input = scanInput.nextInt();

                if (input > 3 || input < 1) {
                    System.out.println("You Can Only Remove 3 Marbles At A Time");
                    continue;
                } 
                marble = marble - input;
                if (marble - input =< 0) {
                    System.out.println("There Are No Marbles Remaining, You Have Won!!");
                    return;
                }
                Thread.sleep(250);
                int cpuAnswer = 4 - input;
                System.out.println("The Computer Has Removed " + cpuAnswer +
                        (cpuAnswer == 1 ? " Marble" : " Marbles"));
                if (marble - cpuAnswer =< 0) {
                    System.out.println("There Are No Marbles Remaining, The Computer Has Won!!");
                    System.out.println("The Computer Has Won");
                    return;
                }

                marble -= cpuAnswer;
            }
        }
    }
}

Now, method separation.

First, let's extract the two main parts into displayHelp and playGame:

import java.util.Scanner;

public class Main {

    public static int input;
    public static int marble = 12;

    public static void main(String[] args) throws InterruptedException {
        displayHelp();
        playGame();
    }

    public void displayHelp() {
        System.out.println("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Time\n"
                + "Your Goal Is To Remove The Last Marble, Marble 1\n"
                + "To Remove A Marble Either Enter 1, 2 or 3\n");
    }

    public void playGame() {
        try (Scanner scanInput = new Scanner(System.in)) {
            while (scanInput.hasNextInput()) {
                System.out.println("There %s %d %s Remaining",
                        (marble == 1 ? "Is" : "Are"),
                        marble,
                        (marble == 1 ? "Marble" : "Marbles"));

                input = scanInput.nextInt();

                if (input > 3 || input < 1) {
                    System.out.println("You Can Only Remove 3 Marbles At A Time");
                    continue;
                } 
                marble = marble - input;
                if (marble - input =< 0) {
                    System.out.println("There Are No Marbles Remaining, You Have Won!!");
                    return;
                }
                Thread.sleep(250);
                int cpuAnswer = 4 - input;
                System.out.println("The Computer Has Removed " + cpuAnswer +
                        (cpuAnswer == 1 ? " Marble" : " Marbles"));
                if (marble - cpuAnswer =< 0) {
                    System.out.println("There Are No Marbles Remaining, The Computer Has Won!!");
                    System.out.println("The Computer Has Won");
                    return;
                }

                marble -= cpuAnswer;
            }
        }
    }

}

Now extract methods into showMarbleCount, getInput, and getCPUInput.

import java.util.Scanner;

public class Main {

    public static int marble = 12;

    public static void main(String[] args) throws InterruptedException {
        displayHelp();
        playGame();
    }

    public void displayHelp() {
        System.out.println("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Time\n"
                + "Your Goal Is To Remove The Last Marble, Marble 1\n"
                + "To Remove A Marble Either Enter 1, 2 or 3\n");
    }

    public void playGame() {
        try (Scanner scanInput = new Scanner(System.in)) {
            while (scanInput.hasNextInput()) {
                showMarbleCount(marble);

                int input = getInput();
                marble -= input;
                if (marble - input =< 0) {
                    System.out.println("There Are No Marbles Remaining, You Have Won!!");
                    return;
                }

                Thread.sleep(250);
                int cpuAnswer = getAndPrintCPUAnswer();
                if (marble - cpuAnswer =< 0) {
                    System.out.println("There Are No Marbles Remaining, The Computer Has Won!!");
                    System.out.println("The Computer Has Won");
                    return;
                }

                marble -= cpuAnswer;
            }
        }
    }

    public void showMarbleCount(int marble) {
        System.out.println("There %s %d %s Remaining",
                (marble == 1 ? "Is" : "Are"),
                marble,
                (marble == 1 ? "Marble" : "Marbles"));
    }

    public int getInput() {
        int input;
        do {
            input = scanInput.nextInt();
            if (input > 3 || input < 1) {
                System.out.println("You Can Only Remove 3 Marbles At A Time");
                continue;
            }
        } while (true);
        return input;
    }

    public int getAndPrintCPUAnswer(int input) {
        int cpuAnswer = 4 - input;
        System.out.println("The Computer Has Removed " + cpuAnswer +
                (cpuAnswer == 1 ? " Marble" : " Marbles"));
        return cpuAnswer;
    }

}
\$\endgroup\$
2
  • \$\begingroup\$ minor pluralization bug: "There Is 1 marbles remaining". You might want to consider suggesting format strings instead of concatenation... \$\endgroup\$
    – Vogel612
    Feb 6, 2016 at 20:06
  • \$\begingroup\$ @Vogel will fix. \$\endgroup\$ Feb 11, 2016 at 21:34
1
\$\begingroup\$

I always like some code kata. So I tried it by myself. But first my suggestions:

  1. After you have the algorithm clear, think about structuring your program. The state pattern is appropriate to represent the game state in every situation.
  2. Extract important constants like the maximum marbles to remove in general
  3. Identify important pieces of calculation and extract them. Calculate the maximum marbles removable at one specific turn.
  4. Do not make the answer of Dr Nim dependent on the user input. This seems to be the easiest way for this case. But if you have more marbles or maybe different rules Dr Nim should evaluate the amount of marbles depending on the remaining marbles. This is a very important semantic issue.
  5. Avoid key words like break or continue. They will hinder you to apply refactorings like "extract method". They break the normal control flow and let the breaking conditions be spread all over the place instead of providing a single point to look at -> the loop header/footer.
  6. The same with return statements. Avoid multiple return statements in long methods, especially in loops. Try to have one return statement at the end of a method.
  7. Provide a proper exit condition for loops. Do not break or continue loops because you mess up the control flow and spread breaking condition all over the place. Extending and refactoring the code will be difficult. Provide the exit condition in the loop header or footer, nowhere else. Other developers should all exit condition see in one place. They should not be forced to collect them through the code.

As I said, I used the state pattern to represent each state of the game. It's verbose and it's not perfect. But maybe you get an idea of the intention of this design decision.

import java.util.Scanner;

public class DrNim {


    public static void main(String[] args) {
        new DrNim().start();
    }


    private static final int MAXIMUM_MARBLES_TO_REMOVE = 3;
    private Scanner scanner;
    private int marbles;

    private State state;


    public synchronized void start() {

        setState(new Start());

        while (!(getState() instanceof End)) {
            getState().execute();
        }

    }


    private State getState() {
        return state;
    }


    private int getMaximumMarblesToRemove() {
        return this.marbles > MAXIMUM_MARBLES_TO_REMOVE ? MAXIMUM_MARBLES_TO_REMOVE : this.marbles;
    }


    private void setState(State state) {
        this.state = state;
        System.out.println(state.getStateInfo());
    }


    private abstract class State {

        public abstract String getStateInfo();

        public abstract void execute();

    }


    private class Start extends State {

        @Override
        public String getStateInfo() {
            StringBuffer sb = new StringBuffer();
            sb.append("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Time\r\n");
            sb.append("Your Goal Is To Remove The Last Marble, Marble 1\r\n");
            sb.append("To Remove A Marble Either Enter 1, 2 or 3\r\n");
            return sb.toString();
        }

        @Override
        public void execute() {
            scanner = new Scanner(System.in);
            marbles = 12;
            setState(new PlayerTurn());
        }

    }


    private class PlayerTurn extends State {

        @Override
        public String getStateInfo() {
            return marbles + " remaining. Players turn...";
        }

        @Override
        public void execute() {
            int marblesToRemove = scanner.nextInt();
            if (marblesToRemove <= 0 || marblesToRemove > getMaximumMarblesToRemove()) {
                setState(new WrongPlayerInput());
            } else {
                setState(new PlayerRemovesMarbles(marblesToRemove));
            }
        }

    }


    private class PlayerRemovesMarbles extends State {

        private int marblesToRemove;

        public PlayerRemovesMarbles(int marblesToRemove) {
            this.marblesToRemove = marblesToRemove;
        }

        @Override
        public String getStateInfo() {
            if (marblesToRemove == 1) {
                return "Player removes 1 marble.";
            } else {
                return "Player removes " + marblesToRemove + " marbles.";
            }
        }

        @Override
        public void execute() {
            try {
                Thread.sleep(250);
            } catch (InterruptedException e) {
            }
            marbles = marbles - marblesToRemove;
            if (marbles == 0) {
                setState(new PlayerWins());
            } else {
                setState(new DrNimTurn());
            }
        }

    }


    private class DrNimTurn extends State {

        @Override
        public String getStateInfo() {
            return marbles + " remaining. Dr Nims turn...";
        }

        @Override
        public void execute() {
            setState(new DrNimRemovesMarbles(calculateMarblesToRemove()));
        }

        private int calculateMarblesToRemove() {
            return marbles % (MAXIMUM_MARBLES_TO_REMOVE + 1);
        }

    }


    private class DrNimRemovesMarbles extends State {

        private int marblesToRemove;

        public DrNimRemovesMarbles(int marblesToRemove) {
            this.marblesToRemove = marblesToRemove;
        }

        @Override
        public String getStateInfo() {
            if (marblesToRemove == 1) {
                return "Dr Nim removes 1 marble.";
            } else {
                return "Dr Nim removes " + marblesToRemove + " marbles.";
            }
        }

        @Override
        public void execute() {
            marbles = marbles - marblesToRemove;
            if (marbles == 0) {
                setState(new DrNimWins());
            } else {
                setState(new PlayerTurn());
            }
        }

    }


    private class WrongPlayerInput extends State {

        @Override
        public String getStateInfo() {
            if (getMaximumMarblesToRemove() == 1) {
                return "You can only remove 1 marble.";
            } else {
                return "You can only remove 1 to " + getMaximumMarblesToRemove() + " marbles.";
            }
        }

        @Override
        public void execute() {
            setState(new PlayerTurn());
        }

    }


    private abstract class End extends State {
    }


    private class PlayerWins extends End {

        @Override
        public String getStateInfo() {
            return "There are no marbles remaining, You have won!!";
        }

        @Override
        public void execute() {
        }

    }


    private class DrNimWins extends End {

        @Override
        public String getStateInfo() {
            return "There are no marbles remaining, Dr Nim has won!!";
        }

        @Override
        public void execute() {
        }

    }

}
\$\endgroup\$
5
  • \$\begingroup\$ I think that "avoid [..] break; or continue;" is bad advice if given in this form. I agree it's important to use them in the correct places and use them as sparsely as possible, but please use them as necessary. A generalizing statement about this could leave the wrong impressions. This applies similarly to "avoid multiple return statements"... This is extremely context-dependent and I'd hope to see a more differentiated discussion of this in your answer. \$\endgroup\$
    – Vogel612
    Feb 6, 2016 at 20:04
  • \$\begingroup\$ I extended my answer. I don't know if you will reject it anyway because of missing "extrem context-dependency" although I now tried to address the "differentiated discussion" point. Here is the thing: We were glad to get rid of goto-statements and got them back through a backdoor. I admit that the goto-statement was introduced with a couple of important usage restrictions. So they couldn't call it "goto" anymore. It reincarnated in break, continue and return. \$\endgroup\$
    – oopexpert
    Feb 7, 2016 at 21:36
  • \$\begingroup\$ But after all it remains a goto-statement. And I will always suggest solutions that will not hinder you to refactor your code or to have breaking conditions for loops at one place. This is definitely not achievable if you use return, break or multiple return-statements. If I think that I have to use such statements I try to restructure my code so it keeps important properties that leave the code easy to modify. And code is easy to modify if I have breaking conditions of loops at one place and parts of a loop or a method can be broken apart with "extract method". \$\endgroup\$
    – oopexpert
    Feb 7, 2016 at 21:47
  • 1
    \$\begingroup\$ By that argument, one should avoid if-statements, Loops and about everything a programming language uses, because at the basic level, it's all just fancy goto statements. \$\endgroup\$
    – Vogel612
    Feb 7, 2016 at 22:45
  • \$\begingroup\$ I hope this isn't a "homicide argument". I tried to unpack the problems with multiple returns, break and continue and provided some metrics that are commonly accepted: Count the conditions that are not mentioned in the loop header/footer but that break a loop. The less you have the better to read and refactor. Count the continues in your loop. The less you have the better to extend and refactor. Count the return statements in a method. The less you have the better to refactor. I do not even know where you are going. Maybe this is off topic and we should discuss this elsewhere. \$\endgroup\$
    – oopexpert
    Feb 7, 2016 at 23:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.