1
\$\begingroup\$

I'm a young and beginner Java programmer and I'm starting to learn again Java (used it in the past years at school) since I want to enter a competition and (hope to) win for getting a job. I wanted to test out what I have learned and start to think about coding in a more object oriented way (coming from C). So I created a simple dice game, wrapping all concepts I have learned (but not applied yet) like abstraction, static methods, encapsulation, interfaces etc..

The game asks you to enter your name and play with him, rolling dice. You roll dice three times and if you get 2 or more same numbers you win 1 coin.

Pretty simple but I'm still confused about many concepts, it can be a problem if I want to "upgrade" or "scale" the game adding more complexity. I don't know how to work better with classes. For example how to "blend" together the Main and GameMenu classes, so I won't have problems adding advanced concepts in the game. Here is the code:

Main class

/*
 *The game will ask your name and to play rolling dices. The game rolls dice three times, if 2 or 
 * more results appear then you earn 1 coin, otherwise you loose 1 coin. After each "round" the game asks you if you 
 * want to continue|increase difficulty (dice with more faces)|exit.
 * 
 */
public class Main {

    public static void main(String[] args) {
        GameMenu.showMenu();
    }

}

Dice class (abstract)

public abstract class Dice {
    private int numFaces;

    public int getNumFaces() {
        return numFaces;
    }

    public void setNumFaces(int numFaces) {
        this.numFaces = numFaces;
    }

    abstract int roll();

}

SixFacesDice class

import java.util.concurrent.ThreadLocalRandom;

public class SixFacesDice extends Dice{

    public SixFacesDice() {
        // TODO Auto-generated constructor stub
        setNumFaces(6);
    }

    @Override
    int roll() {
        // TODO Auto-generated method stub
        return ThreadLocalRandom.current().nextInt(1,6+1);

    }
}

Player class

public class Player {
    private String name;
    private int coins;

    Player(){
        this.name="";
        this.coins=0;
    }

    Player(String name, int coins){
        this.name=name;
        this.coins=coins;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public int getCoins() {
        return coins;
    }

    public void setCoins(int coins) {
        this.coins = coins;
    }

    public void incrementCoins() {
        coins++;
    }

    public void decrementCoins() {
        if(coins>0)
            coins--;
    }
}

GameMenu class

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

//a class for showing the menu to the player
public final class GameMenu {
    static Player player = new Player();

    static void showMenu() {
        Scanner input = new Scanner(System.in);
        boolean playing = true;

        do {
            System.out.print("Welcome! Please enter your name: ");
            player.setName(input.nextLine());
        }
        while(player.getName().isEmpty());

        while(playing) {
                System.out.printf("*************************\n");
                System.out.printf("Welcome back %s, you have %d coins.\n", player.getName(), player.getCoins());
                System.out.println("Enter <any key> to play or write <0> to exit the game.");
                int answer;

                try{
                    answer = Integer.parseInt(input.nextLine());
                }
                catch (Exception e) {
                    answer=1;
                }

                if(answer!=0) {
                    rollMenu();
                }
                else{
                    System.out.println("Goodbye!");
                    playing=false;
                }
        }
    }

    static boolean rollMenu() { //it should be void
        SixFacesDice dice6 = new SixFacesDice();
        int[] results = new int[3];
        boolean win = false;
        int cont=1;

        for(int i=0;i<3;i++) {
            results[i]=dice6.roll();
        }
        //sorting and checking for duplicates (if duplicates are found the player wins)
        Arrays.sort(results);
        for(int i=1;i<results.length;i++) {
            if(results[i] == results[i-1])
                cont++;
        }
        System.out.printf("The numbers rolled are: %d %d %d\n", results[0], results[1], results[2]);

        if(cont>=2)
            win=true;
        else
            win=false;

        if(win) {
            System.out.println("You won! +1 coin");
            player.incrementCoins();
            return true;
        }
        else {
            System.out.println("Sorry you lost!");
            player.decrementCoins();
            return false;
        }
    }
}

Looking to hear your feedback on how I can improve and manage my code.

\$\endgroup\$
  • \$\begingroup\$ " an abstract class refers to something abstract like an animal or a car" That the legacy way. Modern approaches move the concepts (like Car or Animalto interfaces. An interface declares methods a concept provides. Abstact classes provide common behavior for multiple implementations of an interface. But this is also a legacy view to it. Common behavior should be provided by injected dependencies (implementing their own interfaces). \$\endgroup\$ – Timothy Truckle Jan 12 '18 at 11:51
0
\$\begingroup\$

First a minor thing about the code style: don't put else catch while... after a } on a new line. Put it on the same line as the }instead. For example:

    do {
        System.out.print("Welcome! Please enter your name: ");
        player.setName(input.nextLine());
    } while(player.getName().isEmpty());

I noticed the following 2 lines when I was quickly scrolling through your code and thought it was a really nasty bug first:

    }
    while(player.getName().isEmpty());

This is because I didn't immediatly considered that there might be a do{ } block. If the player name was empty and you had just the statement while(true); which would run forever. Putting it on the same line as the } while would immediatly make it clear that the block before it, is part of that while.


Do not provide setters unless if it actually makes sense to change that value.

For example: If I create a player Player me = new Player("Imus", 10); and play the game. Then at the end if I would print out the player, I expect it to still have the name Imus. But since you got a setter for the name field, we can never be sure nobody else has changed my name in the mean time.

Although the coin value does change, you already have an increment and decrement method which we expect to be used. So the setCoins(int coins) is also not needed.

It's far easier to add a setter to a class in case you need it than it is to remove a public method (like a setter) when you don't need it. This is because you may not know which other classes call that method. Especially important on public API's that other people you don't know about may be using.

At least it's a good thing that you got all the fieldsprivate and provided access to them through getters and setters. This is the information hiding that Timothy mentioned in the good coding principles.


Now for your abstract class.

The problem is that you're trying to apply your learned concepts to a really simple example. If you expect a Dice to have a certain number of faces, and be able to roll a number corresponding to one of those faces. Then the roll() method could just as well be implemented in the (abstract) class as well:

public abstract class Dice {
    private int numFaces;

    public Dice(int numFaces) {
        this.numFaces = numFaces;
    }

    public int getNumFaces() {
        return numFaces;
    }

    public int roll() {
        return ThreadLocalRandom.current().nextInt(1,6+1);  
    }
}

(quick note here: no setter, but a constructor that sets the numFaces fields.)

And since this class actually has meaning if it is instanciated on it's own it probably should not be abstract anymore. The only thing the subclass adds is a way to create a six sided die without passing the 6 into the constructor. This might as well just be a factory method instead:

public static Dice createSixSidedDice() {
    return new Dice(6);
}

Either put this method into a specialised DiceFactory class for example. Or just place it inside the Dice class. Added bonus: If you only ever want that someone who uses this class can create 6-sided and 4-sided Dice, you can also make the constructor private and provide the 2 static factory methods to create exactly those 2 types of Dice.


Interface <-> Abstract classes.

What you described was the academic view of abstract classes. In practice we use an abstract class when we have 2 classes that have some common functionality but that common "thing" in itself cannot be used without one of those subclasses. The typical example is indeed something like Cats and Dogs. They have some common Pet traits. So they might have a name, an owner, and they're both able to makeASound() which differs based on if it was a cat or a dog that makes that sound. But you can't do new Animal() and expect a meaningful instance in return. So the classes would look like this:

public abstract class Pet {
    private String name;
    private String owner;

    public Pet(String name, String owner){
        this.name = name;
        this.owner = owner;
    }

    //getters for name and owner

    public abstract String makeASound();
}

public class Cat extends Pet {
    public Cat(String name, String owner){
        super(name, owner);
    }

    @override
    public String makeASound(){
        return "Miauw!";
    }
}

//similar class for Dog but other sound.

When we define an interface we're only making an agreement. For example: If we want to define a Dice as something that you roll() and that then gives back a number without caring how it does that or even how many faces it has, we can define this as the interface:

public interface Dice {
    public int roll();
}

Now we can define all sorts of Dice as long as they implement that roll() method. For example:

public class FairSixSidedDice implements Dice {
    public int roll() {
        return ThreadLocalRandom.current().nextInt(1,7);  
    }
}

Or an unfair Dice that always rolls a six:

public class FairSixSidedDice implements Dice {
    public int roll() {
        return 6;  
    }
}

Or some non-transitive dice to use in some interesting games :)

Note again, that in such a small artificial example it doesn't really shows too much of why this is the best solution.

If you want a real life example of how an interface is an agreement I suggest you take a look at the java AutoClosable interface. This interface says that it doesn't matter what the class actually is, it can be closed. No matter if it's a Port, a Stream, a Channel ...

And since we know it can be closed, it can be used in the try-with-resources construct:

try(Scanner scanner = new Scanner(someInputStream) {

Which means we can be sure that it will be cleaned up when the try finishes, no matter if it's finished normally, or by an exception.


The Main class is perfectly fine as it is now. You got to start somewhere right? An alternative would be to remove that class and put the main method insed the GameMenu class instead.

The issue I have with the GameMenu class is that it actually does 2 major things. It handles the main menu of the game where you input the player and choose to play a game or quit. But it also handles the "roll the same number in 3 dice rolls" game.

This game should probably earn it's own class. And if you add different games they probably should be in their own respective classes. That way, you can just add an option to the main game menu class to play each of the games. And if that option is picked initiate the corresponding game.

This concept is called the SRP (Single Responsibility Principle). Which means that each class should have only 1 main responsibilty (on a certain level of abstraction).

It's hard to tell how much a class is allowed to do so you'll have to get a feel for it on your own. A good rule of thumb is that you should be able to explain what a class does in a sentence or 3. The things you don't mention in those few sentences should follow implicitly. For example:

The GameMenu class shows the user a number of games to play and let's him pick which one or let's him quit the game if he no longer wants to play.

How to display the menu and user input for which game to play are implicit here. How to play that dice game is not.

Note that if the user input was more complicated this might no longer be implicit. In that case "how user input is handled" could fit in it's own class. Then the GameMenu class will call this UserInput class to see which option the user chooses. And then based on that input will start up the corresponding game or quit the program.

\$\endgroup\$
1
\$\begingroup\$

Thanks for sharing your code.


OOP means that you follow certain principles which are (among others):

  • information hiding / encapsulation
  • single responsibility
  • separation of concerns
  • KISS (Keep it simple (and) stupid.)
  • DRY (Don't repeat yourself.)
  • "Tell! Don't ask."
  • Law of demeter ("Don't talk to strangers!")

We inherit from a super class if we extend its behavior. This is: we override a method to do something more and/or something different then the same method in the super class.

You classes Dice and SixFacesDice are a bad example.
Correct singular is Die by the way...

The method roll in SixFacesDice uses the "magic number" 6. This magic number happens to be the same you pass as constructor parameter to it's super class. This means you should better use the super classes variable for the calculation. But this violates information hiding and encapsulation principle. Therefore the implementation of method roll() should be in the super class entirely.

But then your SixFacesDice only configures the super class. Consequently SixFacesDice should not exist and Dice should not be abstract.

The only reason you should have a child class of Dice would be if you want to create a "cheating" die that e.g. always returns a certain number or otherwise "modifies" the randomness of results...

\$\endgroup\$
  • \$\begingroup\$ In modern English, dice is also used as the singular form. Especially here as a class name I would prefer to see the obvious Dice over a more ambiguous Die. \$\endgroup\$ – Imus Jan 12 '18 at 17:08
  • \$\begingroup\$ @Imus I'm not a native speaker, just stumbled over it... :o) \$\endgroup\$ – Timothy Truckle Jan 12 '18 at 17:12

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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