6
\$\begingroup\$

This is pretty much my first "bigger" application besides some "Hello World!" stuff, so I would appreciate critiques to improve my style.

It can be started with Mastermind.play(<difficulty-setting>), with being a number from 1 (easy) to 10 (hard).

public class Mastermind {
    public static void play(int difficulty) {
        int round = 1;
        int[] zahl = getZufallsZahl(difficulty);
        int[] tries = Try(round,difficulty);
        int[] hits = getHits(zahl,tries);
        while (hits[0]!=difficulty) {
            System.out.println("Direct hits: "+hits[0]+"\nIndirect hits: "+hits[1]);
            round++;
            if (round%(int)(Math.random()*10)==0) {
                System.out.println(motivator[(int)(Math.random()*(motivator.length))]);
            }
            tries = Try(round,difficulty);
            hits = getHits(zahl,tries);
        }
        System.out.println("Congratulations! You got it in "+round+" rounds!");
    }
    public static int[] getZufallsZahl(int difficulty) {
        if (difficulty>10) {
            difficulty=10;
        }
        int[] zahl = new int[difficulty];
        int same=1;
        for (int i=0; i<difficulty; i++) {
            // This loop is quite ugly because of the check over the same variable, but I didn't want to use a break with a label (outerloop: for [...] break outerloop;)
            do {
                same=0;
                zahl[i]=(int)(Math.random()*10);
                for (int j=i-1; j>=0; j--) {
                    if (zahl[i]==zahl[j]) {
                        same=1;
                        break;
                    }
                }
            } while (same==1);
        }
        return zahl;
    }



    // Below function is copy pasted! :O (yes I am ashamed for it, but it's "only" a niche-feature, so yeah :/)
    private static String ordinal(int i) {
        String[] sufixes = new String[] { "th", "st", "nd", "rd", "th", "th", "th", "th", "th", "th" };
        switch (i % 100) {
        case 11:
        case 12:
        case 13:
            return i + "th";
        default:
            return i + sufixes[i % 10];
        }
    }
    // Above function is copy pasted! :O



    private static int[] Try(int round, int difficulty) {
        int[] intry = evalTry(getTry(round));
        while (!verifyTry(intry) || intry.length<(difficulty-1) || intry.length>difficulty) {
            System.out.println("Invalid Input!");
            intry = evalTry(getTry(round));
        }
        // This sort of copying of the array makes sure that a 3 element array becomes a 4 element array with a leading 0 to make it easier to process it afterwards.
        int[] tries = new int[difficulty];
        for (int i=(intry.length-1), k=(difficulty-1); i>=0; k--,i--) {
            tries[k]=intry[i];
        }
        return tries;
    }
    private static String getTry(int round) {
        BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
        String in = "";
        System.out.print("This is your "+ordinal(round)+" try: ");
        try {
            in = br.readLine();
        } catch (IOException ex) {
            Logger.getLogger(main.class.getName()).log(Level.SEVERE, null, ex);
        }
        return in;
    }
    private static int[] evalTry(String in) {
        int[] intry = new int[in.length()];
        for (int i=0; i<in.length(); i++) {
            intry[i] = Integer.parseInt(in.substring(i,i+1));
        }
        return intry;
    }
    private static boolean verifyTry(int[] intry) {
        for (int i=0; i<intry.length; i++) {
            for (int j=i-1; j>=0; j--) {
                if (intry[i] == intry[j]) {
                    return false;
                }
            }
        }
        return true;
    }
    private static int[] getHits(int[] zahl,int[] tries) {
        // 0 for direct, 1 for indirect.
        int[] hits = new int[2];
        for (int i=0; i<zahl.length; i++) {
            if (zahl[i]==tries[i]) {
                hits[0]++;
            }
        }
        for (int i=0; i<zahl.length; i++) {
            for (int k=0; k<zahl.length; k++) {
                if (zahl[i]==tries[k] && i!=k) {
                    hits[1]++;
                }
            }
        }
        return hits;
    }
    private static String[] motivator={"You can do it!","Think... Think!","Come on!","You nearly have it!","The force is with you!","FFFFFFFUUUUUUUUUUUU!","This game is stupid anyways...","...","Disregard dumbness, acquire intelligence.","Stop failing, start winning!"};
}
\$\endgroup\$
13
\$\begingroup\$
public class Mastermind {
    public static void play(int difficulty) {
        int round = 1;
        int[] zahl = getZufallsZahl(difficulty);

zahl? What's that?

        int[] tries = Try(round,difficulty);

Usually we have classes capitalized, with methods have the first letter lowercase. Because you don't follow convention, that makes this bit harder to read

        int[] hits = getHits(zahl,tries);

Ok, but tries implies multiple tries. This is a single attempt, and the multiplicity is in the code assignment.

        while (hits[0]!=difficulty) {
            System.out.println("Direct hits: "+hits[0]+"\nIndirect hits: "+hits[1]);
            round++;
            if (round%(int)(Math.random()*10)==0) {
                System.out.println(motivator[(int)(Math.random()*(motivator.length))]);
            }
            tries = Try(round,difficulty);
            hits = getHits(zahl,tries);

You've repeated code before and in the loop. If you use a break in the middle of the loop, you can avoid that. (If somebody told you not to use break, they were wrong).

        }
        System.out.println("Congratulations! You got it in "+round+" rounds!");
    }

    public static int[] getZufallsZahl(int difficulty) {
        if (difficulty>10) {
            difficulty=10;
        }
        int[] zahl = new int[difficulty];
        int same=1;
        for (int i=0; i<difficulty; i++) {
            // This loop is quite ugly because of the check over the same variable, but I didn't want to use a break with a label (outerloop: for [...] break outerloop;)
            do {
                same=0;
                zahl[i]=(int)(Math.random()*10);
                for (int j=i-1; j>=0; j--) {
                    if (zahl[i]==zahl[j]) {
                        same=1;
                        break;
                    }
                }
            } while (same==1);
        }

Yeah... Don't do that. Instead:

  1. Start with an array [0,1,2,3,4,5,6,7,8,9]
  2. Shuffle the array
  3. Shrink the array to the correct size.

That'll be easier to follow

        return zahl;
    }



    // Below function is copy pasted! :O (yes I am ashamed for it, but it's "only" a niche-feature, so yeah :/)

Nothing wrong with copy-pasting. You should take advantage of code that other people wrote as much as possible. The problem with copy-paste is multiple copies of the same code in your program, not taking working code from elsewhere.

    private static String ordinal(int i) {
        String[] sufixes = new String[] { "th", "st", "nd", "rd", "th", "th", "th", "th", "th", "th" };
        switch (i % 100) {
        case 11:
        case 12:
        case 13:
            return i + "th";
        default:
            return i + sufixes[i % 10];
        }
    }
    // Above function is copy pasted! :O



    private static int[] Try(int round, int difficulty) {
        int[] intry = evalTry(getTry(round));

intry? What does that mean? Try to pick helpful variable names

        while (!verifyTry(intry) || intry.length<(difficulty-1) || intry.length>difficulty) {
            System.out.println("Invalid Input!");
            intry = evalTry(getTry(round));
        }

As before, don't duplicate the contents of the loop before the loop.

        // This sort of copying of the array makes sure that a 3 element array becomes a 4 element array with a leading 0 to make it easier to process it afterwards.

What? That's a confusing way to explain what you are doing here. Just say that you are filling the array with leading zeros.

        int[] tries = new int[difficulty];
        for (int i=(intry.length-1), k=(difficulty-1); i>=0; k--,i--) {
            tries[k]=intry[i];

What if intry is too long rather then too short?

        }
        return tries;
    }
    private static String getTry(int round) {
        BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
        String in = "";
        System.out.print("This is your "+ordinal(round)+" try: ");
        try {
            in = br.readLine();
        } catch (IOException ex) {
            Logger.getLogger(main.class.getName()).log(Level.SEVERE, null, ex);

For quick and dirty application, I suggest throw new RuntimeException(ex); log messages are liable to be missed.

        }
        return in;
    }
    private static int[] evalTry(String in) {
        int[] intry = new int[in.length()];
        for (int i=0; i<in.length(); i++) {
            intry[i] = Integer.parseInt(in.substring(i,i+1));
        }
        return intry;
    }
    private static boolean verifyTry(int[] intry) {
        for (int i=0; i<intry.length; i++) {
            for (int j=i-1; j>=0; j--) {

Why do you keep making your loops backwards for no reason?

                if (intry[i] == intry[j]) {
                    return false;
                }
            }
        }
        return true;
    }
    private static int[] getHits(int[] zahl,int[] tries) {
        // 0 for direct, 1 for indirect.
        int[] hits = new int[2];

I'd suggest storing the hits in two variables rather then as a two element array. Just stick them in the array when you return.

        for (int i=0; i<zahl.length; i++) {
            if (zahl[i]==tries[i]) {
                hits[0]++;
            }
        }
        for (int i=0; i<zahl.length; i++) {
            for (int k=0; k<zahl.length; k++) {
                if (zahl[i]==tries[k] && i!=k) {
                    hits[1]++;
                }
            }
        }
        return hits;
    }

    private static String[] motivator={"You can do it!","Think... Think!","Come on!","You nearly have it!","The force is with you!","FFFFFFFUUUUUUUUUUUU!","This game is stupid anyways...","...","Disregard dumbness, acquire intelligence.","Stop failing, start winning!"};

The name motivator should probably be plural.

}
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot! I updated my question with the changed code I wrote. I have a question thought, is it really better if I make an infinite loop in which I break out of instead of just declaring the variables beforehand and using them in the Statement for the loop? Or did I misunderstand you on that part? ("You've repeated code before and in the loop.") \$\endgroup\$ – halfbit Mar 30 '12 at 10:09
  • 1
    \$\begingroup\$ @Mastermind, I have to admit people will disagree on whether the infinite loop is better. I think it is better to have an infinite loop that you break out of. Its better if you can avoid using break, but its more important to avoid duplication. \$\endgroup\$ – Winston Ewert Mar 30 '12 at 13:48

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.