5
\$\begingroup\$

First of all, I have my basic main method which does pretty much nothing:

public static void main(String[] args) {
    higherOrLower HOL = new higherOrLower();
    HOL.playGame();

}

Then I have the class and methods where all the action happens:

import java.util.Arrays;
import java.util.Scanner;

public class higherOrLower {

int set[] = new int[8];
private int flips = 1;

public higherOrLower() {
    deck deck = new deck();
    for (int i = 0; i < 8; i++) {
        set[i] = deck.draw();
    }
}

public void playGame() {
    Scanner sc = new Scanner(System.in);
    paint();
    for (int i = 0; i < 7; i++) {
        System.out.println("Higher or Lower?");
        String an = sc.nextLine();
        switch (an) {
            case "Higher":
            case "higher":
            case "H":
            case "h":

                if (set[i] > set[i + 1]) {
                    System.out.println("You Lose");
                    endGame();
                } 

                break;
            case "Lower":
            case "lower":
            case "L":
            case "l":

                if (set[i] < set[i + 1]) {
                    System.out.println("You Lose");
                    endGame();
                }

                break;
            default:
                System.out.println("Please enter one of the following \nHigher\nhigher\nH\nh\nLower\nlower\nL\nl");
                i--;
                break;
        }
        flips++;
        paint();
    }

    System.out.println("You win!");
    endGame();
}

public static void playAgain() {
    higherOrLower HOL = new higherOrLower();
    HOL.playGame();
}

public void flip() {
    flips++;
}

void endGame() {
    Scanner sc = new Scanner(System.in);
    System.out.println("Play again?");
    boolean done = false;
    while(!done){
    String ans = sc.nextLine();

    switch (ans) {
        case "Yes":
        case "yes":
        case "Y":
        case "y":
            done = true;
            playAgain();
            break;
        case "No":
        case "no":
        case "N":
        case "n":
            done = true;
            System.exit(0);
            break;
        default:
            System.out.println("Please enter one of the following \nYes\nyes\nY\ny\nNo\nno\nN\nn");
            break;
    }
    }
}

public void paint() {
    if (flips == 1) {
        for (int i = 0; i < 8; i++) {
            graphics(set[0], i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.println("");
        }

        System.out.println("");
        System.out.println("");

        for (int i = 0; i < 8; i++) {
            graphics(-1, i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.println("");
        }
    }
    if (flips == 2) {
        for (int i = 0; i < 8; i++) {
            graphics(set[0], i);
            System.out.print("  ");
            graphics(set[1], i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.println("");
        }

        System.out.println("");
        System.out.println("");

        for (int i = 0; i < 8; i++) {
            graphics(-1, i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.println("");
        }

    }
    if (flips == 3) {
        for (int i = 0; i < 8; i++) {
            graphics(set[0], i);
            System.out.print("  ");
            graphics(set[1], i);
            System.out.print("  ");
            graphics(set[2], i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.println("");
        }

        System.out.println("");
        System.out.println("");

        for (int i = 0; i < 8; i++) {
            graphics(-1, i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.println("");
        }
    }
    if (flips == 4) {
        for (int i = 0; i < 8; i++) {
            graphics(set[0], i);
            System.out.print("  ");
            graphics(set[1], i);
            System.out.print("  ");
            graphics(set[2], i);
            System.out.print("  ");
            graphics(set[3], i);
            System.out.println("");
        }

        System.out.println("");
        System.out.println("");

        for (int i = 0; i < 8; i++) {
            graphics(-1, i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.println("");
        }
    }
    if (flips == 5) {
        for (int i = 0; i < 8; i++) {
            graphics(set[0], i);
            System.out.print("  ");
            graphics(set[1], i);
            System.out.print("  ");
            graphics(set[2], i);
            System.out.print("  ");
            graphics(set[3], i);
            System.out.println("");
        }

        System.out.println("");
        System.out.println("");

        for (int i = 0; i < 8; i++) {
            graphics(set[4], i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.println("");
        }
    }
    if (flips == 6) {
        for (int i = 0; i < 8; i++) {
            graphics(set[0], i);
            System.out.print("  ");
            graphics(set[1], i);
            System.out.print("  ");
            graphics(set[2], i);
            System.out.print("  ");
            graphics(set[3], i);
            System.out.println("");
        }

        System.out.println("");
        System.out.println("");

        for (int i = 0; i < 8; i++) {
            graphics(set[4], i);
            System.out.print("  ");
            graphics(set[5], i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.println("");
        }
    }
    if (flips == 7) {
        for (int i = 0; i < 8; i++) {
            graphics(set[0], i);
            System.out.print("  ");
            graphics(set[1], i);
            System.out.print("  ");
            graphics(set[2], i);
            System.out.print("  ");
            graphics(set[3], i);
            System.out.println("");
        }

        System.out.println("");
        System.out.println("");

        for (int i = 0; i < 8; i++) {
            graphics(set[4], i);
            System.out.print("  ");
            graphics(set[5], i);
            System.out.print("  ");
            graphics(set[6], i);
            System.out.print("  ");
            graphics(-1, i);
            System.out.println("");
        }
    }
    if (flips == 8) {
        for (int i = 0; i < 8; i++) {
            graphics(set[0], i);
            System.out.print("  ");
            graphics(set[1], i);
            System.out.print("  ");
            graphics(set[2], i);
            System.out.print("  ");
            graphics(set[3], i);
            System.out.println("");
        }

        System.out.println("");
        System.out.println("");

        for (int i = 0; i < 8; i++) {
            graphics(set[4], i);
            System.out.print("  ");
            graphics(set[5], i);
            System.out.print("  ");
            graphics(set[6], i);
            System.out.print("  ");
            graphics(set[7], i);
            System.out.println("");
        }
    }
}

void graphics(int card, int row) {
    switch (card) {

        case 1:
            switch (row) {

                case 1:
                    System.out.print(" ________ ");
                    break;
                case 2:
                    System.out.print("|A.......|");
                    break;

                case 3:
                    System.out.print("|........|");

                    break;
                case 4:
                    System.out.print("|....֍...|");

                    break;
                case 5:
                    System.out.print("|........|");

                    break;
                case 6:
                    System.out.print("|.......A|");
                    break;
                case 7:
                    System.out.print("|________|");

                    break;
            }
            break;
        case 2:
            switch (row) {

                case 1:
                    System.out.print(" ________ ");
                    break;
                case 2:
                    System.out.print("|2.......|");
                    break;

                case 3:
                    System.out.print("|........|");

                    break;
                case 4:
                    System.out.print("|....֍...|");

                    break;
                case 5:
                    System.out.print("|........|");

                    break;
                case 6:
                    System.out.print("|.......2|");
                    break;
                case 7:
                    System.out.print("|________|");

                    break;
            }
            break;

        case 3:
            switch (row) {

                case 1:
                    System.out.print(" ________ ");
                    break;
                case 2:
                    System.out.print("|3.......|");
                    break;

                case 3:
                    System.out.print("|........|");

                    break;
                case 4:
                    System.out.print("|....֍...|");

                    break;
                case 5:
                    System.out.print("|........|");

                    break;
                case 6:
                    System.out.print("|.......3|");
                    break;
                case 7:
                    System.out.print("|________|");

                    break;
            }
            break;
        case 4:
            switch (row) {

                case 1:
                    System.out.print(" ________ ");
                    break;
                case 2:
                    System.out.print("|4.......|");
                    break;

                case 3:
                    System.out.print("|........|");

                    break;
                case 4:
                    System.out.print("|....֍...|");

                    break;
                case 5:
                    System.out.print("|........|");

                    break;
                case 6:
                    System.out.print("|.......4|");
                    break;
                case 7:
                    System.out.print("|________|");

                    break;
            }
            break;
        case 5:
            switch (row) {

                case 1:
                    System.out.print(" ________ ");
                    break;
                case 2:
                    System.out.print("|5.......|");
                    break;

                case 3:
                    System.out.print("|........|");

                    break;
                case 4:
                    System.out.print("|....֍...|");

                    break;
                case 5:
                    System.out.print("|........|");

                    break;
                case 6:
                    System.out.print("|.......5|");
                    break;
                case 7:
                    System.out.print("|________|");

                    break;
            }
            break;
        case 6:
            switch (row) {

                case 1:
                    System.out.print(" ________ ");
                    break;
                case 2:
                    System.out.print("|6.......|");
                    break;

                case 3:
                    System.out.print("|........|");

                    break;
                case 4:
                    System.out.print("|....֍...|");

                    break;
                case 5:
                    System.out.print("|........|");

                    break;
                case 6:
                    System.out.print("|.......6|");
                    break;
                case 7:
                    System.out.print("|________|");

                    break;
            }
            break;
        case 7:
            switch (row) {

                case 1:
                    System.out.print(" ________ ");
                    break;
                case 2:
                    System.out.print("|7.......|");
                    break;

                case 3:
                    System.out.print("|........|");

                    break;
                case 4:
                    System.out.print("|....֍...|");

                    break;
                case 5:
                    System.out.print("|........|");

                    break;
                case 6:
                    System.out.print("|.......7|");
                    break;
                case 7:
                    System.out.print("|________|");

                    break;
            }
            break;
        case 8:
            switch (row) {

                case 1:
                    System.out.print(" ________ ");
                    break;
                case 2:
                    System.out.print("|8.......|");
                    break;

                case 3:
                    System.out.print("|........|");

                    break;
                case 4:
                    System.out.print("|....֍...|");

                    break;
                case 5:
                    System.out.print("|........|");

                    break;
                case 6:
                    System.out.print("|.......8|");
                    break;
                case 7:
                    System.out.print("|________|");

                    break;
            }
            break;
        case 9:
            switch (row) {

                case 1:
                    System.out.print(" ________ ");
                    break;
                case 2:
                    System.out.print("|9.......|");
                    break;

                case 3:
                    System.out.print("|........|");

                    break;
                case 4:
                    System.out.print("|....֍...|");

                    break;
                case 5:
                    System.out.print("|........|");

                    break;
                case 6:
                    System.out.print("|.......9|");
                    break;
                case 7:
                    System.out.print("|________|");

                    break;
            }
            break;
        case 10:
            switch (row) {

                case 1:
                    System.out.print(" ________ ");
                    break;
                case 2:
                    System.out.print("|10......|");
                    break;

                case 3:
                    System.out.print("|........|");

                    break;
                case 4:
                    System.out.print("|....֍...|");

                    break;
                case 5:
                    System.out.print("|........|");

                    break;
                case 6:
                    System.out.print("|......10|");
                    break;
                case 7:
                    System.out.print("|________|");

                    break;
            }
            break;
        case 11:
            switch (row) {

                case 1:
                    System.out.print(" ________ ");
                    break;
                case 2:
                    System.out.print("|J.......|");
                    break;

                case 3:
                    System.out.print("|........|");

                    break;
                case 4:
                    System.out.print("|....֍...|");

                    break;
                case 5:
                    System.out.print("|........|");

                    break;
                case 6:
                    System.out.print("|.......J|");
                    break;
                case 7:
                    System.out.print("|________|");

                    break;
            }
            break;
        case 12:
            switch (row) {

                case 1:
                    System.out.print(" ________ ");
                    break;
                case 2:
                    System.out.print("|Q.......|");
                    break;

                case 3:
                    System.out.print("|........|");

                    break;
                case 4:
                    System.out.print("|....֍...|");

                    break;
                case 5:
                    System.out.print("|........|");

                    break;
                case 6:
                    System.out.print("|.......Q|");
                    break;
                case 7:
                    System.out.print("|________|");

                    break;
            }
            break;
        case 13:
            switch (row) {

                case 1:
                    System.out.print(" ________ ");
                    break;
                case 2:
                    System.out.print("|K.......|");
                    break;

                case 3:
                    System.out.print("|........|");

                    break;
                case 4:
                    System.out.print("|....֍...|");
                    break;
                case 5:
                    System.out.print("|........|");
                    break;
                case 6:
                    System.out.print("|.......K|");
                    break;
                case 7:
                    System.out.print("|________|");
                    break;
            }
            break;
        case -1:
            switch (row) {

                case 1:
                    System.out.print(" ________ ");
                    break;
                case 2:
                    System.out.print("|        |");
                    break;

                case 3:
                    System.out.print("|\\//\\/\\/\\|");

                    break;
                case 4:
                    System.out.print("|/\\/\\/\\/\\|");

                    break;
                case 5:
                    System.out.print("|\\//\\/\\/\\|");

                    break;
                case 6:
                    System.out.print("|/\\/\\/\\/\\|");
                    break;
                case 7:
                    System.out.print("|________|");

                    break;
            }
            break;
        case 0:
            System.out.println("000000000000000000000000000000000000000");
        default:
            System.out.println("error?");
    }

}

}

And after that I have one other class, the deck:

public class deck {

public int deck[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13};

public deck() {
    shuffle();
}

private void shuffle() {
    Random rnd = ThreadLocalRandom.current();
    for (int i = deck.length - 1; i > 0; i--) {
        int index = rnd.nextInt(i + 1);
        int a = deck[index];
        deck[index] = deck[i];
        deck[i] = a;
    }
}

public int draw(){
    int top = 0;
    for (int i = 0; i < 52; i++) {
        if(deck[i]!=0){
            top = deck[i];
            deck[i] = 0;
            break;
        }

    }
    return top;
}

@Override
public String toString(){
    String temp = "[";
    for (int i = 0; i < 51; i++) {
        temp = temp + deck[i]+",";
    }
    temp = temp + deck[51] + "]";
    return temp;
}

}

What do you think? Could I have done anything better?

\$\endgroup\$
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Feel free to post a follow-up question instead, but I'd give this question at least 2 more days before doing so. \$\endgroup\$ – Mast Aug 4 '18 at 15:52
  • \$\begingroup\$ @Mast OK. Sorry, this is my first post in this community . \$\endgroup\$ – Clint Aug 4 '18 at 15:56
  • \$\begingroup\$ Don't worry, it's not immediately obvious to most people new to the site. So far you've been doing fine, feel free to stick around :-) \$\endgroup\$ – Mast Aug 4 '18 at 16:00
3
\$\begingroup\$

I can't give special improvements for performance in general, but I can give you a first impression of your code and some improvements I would take:

playGame and endGame Method

First for a plain true/false or a/b decision based on terminal input like you use it in the methods playGame() and endGame() I would use equalsIgnoreCase() or something like that:

So I would turn

switch (an)
{
    case "Higher":
    case "higher":
    case "H":
    case "h":
    {
        break;
    }

    ...
}

in something like

if(an.equalsIgnoreCase("higher") || an.toLowerCase().startsWith("h"))
{
    ...
}
else if(an.equalsIgnoreCase("lower") || an.toLowerCase().startsWith("l"))
{
    ...
}
else
{
    ...
}

Maybe it is a personal opinion, but I think it is more readable...

paint Method

Moreover I wouln't call System.out.print() all the time like you do it in your paint() method:

I would say the method graphics() shouldn't print the card string itself to output stream, instead it should just return the string...

String graphics(int card, int row) { ... }

This increases the reuse of this method and makes it possible to format your output in your paint() method...

For example instead of writing

for (int i = 0; i < 8; i++)
{
    graphics(set[0], i);
    System.out.print("  ");
    graphics(set[1], i);
    System.out.print("  ");
    graphics(set[2], i);
    System.out.print("  ");
    graphics(set[3], i);
    System.out.println("");
}

I would write something like this

for (int i = 0; i < 8; i++)
{
    System.out.printf("%s  %s  %s  %s%n", graphics(set[0], i), graphics(set[1], i), graphics(set[2], i), graphics(set[3], i));
}

graphics Method

In addition, I would use a collection to store the string parts of each card something like a Map in your graphics() method...

Map <Integer, List <String>> cards; // Each card is structured in a list of rows...

To access a specific row of a card to print it you just write something like

String row = cards.get(numberOfCard).get(numberOfRow);

The usage of such a collection will simplify your graphics() method a lot!

This are also a few suggestions, in addition I would write a extra class for Card and seperate more the logic from the interface also for increasing readability... But this are just hints...

\$\endgroup\$
  • \$\begingroup\$ Thank you. Those are definitely some things I will look into/change. \$\endgroup\$ – Clint Aug 4 '18 at 15:18
4
\$\begingroup\$

Here are my notes:

  • I think it's good to have consistency in the way things are named: I noticed that you've named one variable HOL and one deck. Even if something is an acronym, I would opt to keep consistency and start the name with lowercase if you do that for other variables. In this instance, I'd actually opt to rename HOL to something that is more descriptive independent of looking at its type, such as higherOrLowerGame. It's a bit longer, but it reads better and you could complement it by changing playGame to play, thus allowing you to write higherOrLowerGame.play(), which reads nicely.

  • Additionally, I personally like to use Pascal Case for classes and Lower Camel Case for variables: it can help clarify situations like deck deck = new deck() by making a clear distinction between the decks: Deck deck = new Deck() looks better and clearer in my opinion.

  • I'd opt to de-hardcode this as much as possible, starting with the higherOrLower constructor. It has a for loop iterate from 0 to 7, but the boundary of i < 8 would be better if it was i < set.length.

  • For dealing with all the variations of input, I would have an ArrayList for each input which have acceptable input values inserted into them at instantiation. Then, you can have if (acceptableHigherInputs.contains(an)) { (and similar for each of the possible inputs: higher, lower, yes, no) instead of the large switch block. It also makes it easy if you ever want to add other possible inputs for each function.

  • At first glance, I'd recommend changing the name of playAgain, as it is static, so playAgain doesn't make sense as it can still be called if there was no game in the first place. Looking at the contents of the function, I'd actually recommend removing it altogether. Instead, create a method called setup which generates a deck and resets flips, then replace the content of the constructor with a call to that method. Then, if the player wants to play again, you just need to call "setup" and then playGame instead of having the playAgain method create a new object.

  • Remove the flip method: it doesn't add any value, as it just increments a variable which can be done just as easily without the function, and it doesn't look like you have even called the function once in your code.

  • Use a LinkedList<Integer> (from java.util.LinkedList) in place of an integer array for your deck class. Firstly, from your draw method it looks like you're trying to return the first non-zero value in the array and then set it to zero. The LinkedList's pop method will work better for you: it removes and returns the first value in the list. Thus, you can replace the content of your draw method with return list.pop();. Additionally, it has a toString method that provides formatting exactly how you have implemented your toString method, so you can replace the toString's content with return list.toString();. Another benefit of using a LinkedList is that it allows you to use the Collections.shuffle method (from java.util.Collections), so you can remove your shuffle implementation.

  • If you end up using a LinkedList, you'll need to add values to it in either your constructor or a setup method: you can use a for loop to do this very easily, given the set of values that your array contains. Additionally, I can't see a point of having 52 values in it, since they are repeated values and I don't believe the size of the array impacts the program at all: it shouldn't affect the odds of the number generation if you leave it at 13 values.

EDIT:

Just some additional notes:

  • Try to make your code as self-descriptive as possible: part of this is through variable naming. Names like an and sc are unclear on their own, however userInput and inputScanner are much clearer.

  • Only have one exit point for the program if you can help it. Don't call System.exit without reason: just let the program reach its end naturally. In this instance, if you don't call System.exit, the program will reach the end of main and terminate regardless.

\$\endgroup\$
  • \$\begingroup\$ For your first 2 points, I agree, and thank you! On #3 It could work with i < set.length, but what would be the point/advantage? #4 could help clean the code up. all good there and thanks. #5 I couldn't agree with you more. Original I had the game working in a different way and meant to change the static but forgot to. And I like the setup method Idea. Same with the flip method. It used to have a different use, but I changed it. The linked list sounds like a good idea too. The only thing is , when you play the game IRL, you may have duplicate cards ie king, then another king. \$\endgroup\$ – Clint Aug 4 '18 at 20:06
  • 2
    \$\begingroup\$ No problem! Regarding your question on point 3, take a look at the following: stackoverflow.com/questions/47882/… \$\endgroup\$ – VortixDev Aug 4 '18 at 20:17
  • 2
    \$\begingroup\$ Regarding the duplicate entries, I understand now. There is the alternative method of maintaining two lists (or one list and one array), where one list is used for the randomised deck and the other is a list of distinct possibilities for each entry: basically, you generate the list by picking a random value from the distinct array for each element. Though, having duplicated values and a single list is an easier and suitable approach for the given task. \$\endgroup\$ – VortixDev Aug 4 '18 at 20:21
2
\$\begingroup\$

The paint and graphics methods could use an upgrade. I reworked them and eliminated 661 lines of code to try to handle the DRY principle.

private void paint() {
    int max = 8;
    for (int row = 0; row < max; row++) {
        for (int i = 0; i < flips; i++) {
            System.out.print(getCard(Integer.toString(set[i]), row) + "  ");
        }
        for (int j = flips; j < max; j++) {
            System.out.print(getEmptyCard(row) + " ");
        }
        System.out.println();
    }
}

private String getEmptyCard(int row) {
    String horizontalBorder = " -------- ";
    String a = "|/\\/\\/\\/\\|";
    String b = "|\\//\\/\\/\\|";
    String c = "|        |";
    if (row == 0 || row == 7) return horizontalBorder;
    else if (row == 1 || row == 6) return c;
    else if (row == 2 || row == 4) return a;
    else return b;
}

private String getCard(String card, int row) {
    String horizontalBorder = " -------- ";
    String body = "|        |";
    String top = String.format("|%-8s|", card);
    String bottom = String.format("|%8s|", card);
    if (row == 0 || row == 7) return horizontalBorder;
    else if (row == 1) return top;
    else if (row == 6) return bottom;
    else return body;
}

I also worked out another example of your Deck class. Instead of hardcoding the values in an array you could loop it into an ArrayList. Also, Java has a built in function to shuffle collections.

public class Deck {

    private List<Integer> deck;

    public Deck() {
        initDeck();
    }

    private void initDeck() {
        deck = new ArrayList<>();
        for (int i = 0; i < 2; i++) {
            for (int j = 1; j <= 13; j++) {
                deck.add(j);
            }
        }
        Collections.shuffle(deck);
    }

    public int draw() {
        int top = deck.get(deck.size() - 1);
        deck.remove(deck.size() - 1);
        return top;
    }

}
\$\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.