Good or bad oop-practise?

Question

Hi! I have a working program which where you play a game named Nim against the computer. What I wonder is if my solution when it comes to inherit the classes is good or bad oop-practise. This is how the main-method looks like:

public static void main(String[] args) {
        // TODO code application logic here
        Game game = new Game();
        User user = new User(game);
        Computer computer = new Computer(game);
        game.Play(user, computer);
    }

The program contains of 4 classes:

Nim, which is a superclass and abstract
User, which represents a user playing the game
Computer, which represents the computer and has a easy algoritm for the playing the game
Game, which is a subclass and extends of Nim. Game contains all the boolean functions which works like rules in the game.

Here are the classes:

abstract public class Nim {

    private int pile;

    public int getPile(){
        return pile;
    }

    public void setPile(int pile){
        this.pile = pile;
    }
}

public class User {

    private Game game;
    public User(Game game){
        this.game = game;
    }

    public void userRemove(int x){
        System.out.println("User removes " + x + " from pile");
        game.setPile(game.getPile()-x);
        System.out.println("Left from pile: " + game.getPile());
    }

    public void userTryRemove(){
        Scanner input = new Scanner(System.in);
        System.out.println("User will remove from pile");
        System.out.printf("How Many will you remove? ");
        int x = input.nextInt();
        while ( !game.checkRemove(x) ){
            System.out.println("- No valid input");
            System.out.printf("How Many will you remove? ");
            x = input.nextInt();
        }
        userRemove(x);
    }
}

public class Computer {

    private Game game;
    public Computer ( Game game ){
        this.game = game;
    }

    public void computerRemove(int x){
        System.out.println("Computer removes " + x + " from pile");
        game.setPile(game.getPile()-x);
        System.out.println("Left from pile: " + game.getPile());
    }

    public void computerTryRemove(){
        System.out.println("Computer will remove from pile");
        int x = game.generateRandomNumber(); 
        while ( !game.checkRemove(x) ){
            System.out.println("- No valid input");
            x = game.generateRandomNumber(); 
        }
        computerRemove(x);
    }
}

import java.util.concurrent.ThreadLocalRandom;
import java.util.Scanner;
/**
 *
 * @author Björn Norén
 */
public class Game extends Nim {

    //Kollar om högen = 1
    public boolean checkPile(){
        boolean ok;
        if ( getPile() <= 1 )
            ok = false;
        else 
            ok = true;
        return ok;      
    }

    //Kollar om remove är större än halva högen
    public boolean checkRemove(int x){
        boolean ok;
        if ( x > getPile() / 2 || x <= 0 )
            ok = false;
        else
            ok = true;
        return ok;
    }

    public boolean checkRemove(String x){
        return false;
    }

    public int generateRandomNumber(){
        int min = 1;
        int max = getPile();
        int randomNum = ThreadLocalRandom.current().nextInt(min, max + 1);

        return randomNum;
    }

    public void setStartPile(){
        Scanner input = new Scanner(System.in);
        System.out.println("Set the amount of pile: ");
        int x = input.nextInt();
        setPile(x);
    }

    public void Play(User user, Computer computer){
        setStartPile();
        System.out.println("Pile: " + getPile());
        while ( checkPile() ){         
            user.userTryRemove();
            if( checkPile() )
                computer.computerTryRemove();
            else {
                System.out.println("The pile is empty! Computer won!");
                break;
            }
        }  
        System.out.println("The pile is empty! User won!!");
    }


    public static void main(String[] args) {
        // TODO code application logic here
        Game game = new Game();
        User user = new User(game);
        Computer computer = new Computer(game);
        game.Play(user, computer);
    }
}

Answer 1 · 2017-02-13T11:24:49Z

February 13, 2017 11:24am

1) don't name your methods with upper-case letters public void Play(User user, Computer computer){ -> public void play(User user, Computer computer){

I can give usefull advice about your question good OOP practice is to imagine what happens if you add for examle: a) more games b) more users c) more computers d) user switch between games functionality e) user switch between same game on different comps and so on

Like for this task you don't have much OOP involved and it looks ok but all classes hardwired on each other.

Try reading some patterns in java to understand this stuff

Answer 2 · 2017-09-06T20:39:24Z

September 6, 2017 8:39pm

Not sure if you're still working on this, but I thought I'd add a few thoughts regarding OOP (object oriented programming) for you or anyone else who might look at this question later:

1) Like Semen mentioned, your classes are "hard-wired" together. The Game class takes the User and Computer as parameters to its play() method AND both of them take the Game object, too. You could make it so the Game object just takes the other two as parameters, by sending the Game object to each method called in the User and Computer objects like this:

In Game,

public void play( User user, Computer computer ){
        // ...
        while ( checkPile() ){         
            user.userTryRemove( this ); // this references the Game object because this method is inside it
            if( checkPile() )
                computer.computerTryRemove( this ); // here again, same thing
            // ...
        }  
        // ...
    }

In User (and similarily in Computer):

public void userTryRemove( Game game ){ // add the game as a parameter here instead of in the constructor
        // ...
        while ( !game.checkRemove(x) ){
            // ...
        }
        // ...
    }

and then in main():

public static void main(String[] args) {
        Game game = new Game();
        User user = new User();
        Computer computer = new Computer();
        game.play( user, computer );
    }

This allows you to decouple the classes some - you can easily change the game (and the rules) by just switching out the one object. It also prevent mistakes if you someday have multiple games - you can't accidentally start a different game with game.play() than the one you assigned to User or Computer, which would break the program.

2) Think about your inheritance some:

Does the Nim abstract class represent a real concept? Like are there many "Nim" games that use the same pile, but have different rules? If so, it makes sense they share the pile methods. Otherwise, there might be a better way to approach that. And if there are different variants of the base game, are there more rules or setup steps that are shared between all "Nim" games that could be added to the abstract class?
User and Computer are different types of Players so you probably want them to inherit from an abstract class called Player and then have different implementations for the methods - User asking for input, and Computer deciding based on the simple algorithm. This would allow for the logical and easy addition of other algorithms like HardComputer or RandomComputer, etc. or multiplayer games later - instead of a User and a Computer, Game.play() would look for two Player parameters that could be Users or Computer algorithms (since they would each extend Player). This is an advantage of good OOP - flexibility for changes later on in development. This is also a reason you might consider a more descriptive name for Computer - maybe EasyComputer or SimpleAlgorithm. Since you create the object with Computer computer = new SimpleAlgorithm();, you don't necessarily need Computer in name of the object.

3) Not really OOP, but still worth noting, your Game.checkRemove( String x ) method is essentially an input validator. This should probably either be done in the User class - where the input is received - OR in a separate class that deals with input validation (or utility functions). I don't think it fits in with the concept of a Game class that is primarily about the rules of the game. That is UNLESS different games with different rules would have different kinds of acceptable input.

4) On the topic of different Games with different rules, you might consider a more descriptive name for Game, too. Maybe TraditionalRules or ModernRules. Again, remember you don't have to include the abstract class name in the implementing class because it will be obvious in code: Game game = new TraditionalRules();.

5) Finally, consider using Interfaces instead of Abstract Classes if they fit better:

Interfaces generally define a contract for a group of classes to follow together so they can be used interchangeably.
Abstract Classes define shared methods for a group of classes, some of which will probably be identical and can be implemented in the abstract class, but others will be unique for each subclass and left abstract.

It makes sense for Nim to be an abstract class - although it should have abstract methods for any methods that must be in every Game. On the other hand, when you add Player it should probably be an interface. It should define a contract for all of the methods that must be defined in User and Computer objects even though those methods will probably be different in each class' implementation.

Good luck! Good OOP is difficult because there are a lot of ways to make things work, but you have to try to find one that makes logical and programmatic sense and find names that are meaningful and understandable for everyone working on the project (even if it's just you, you want to be able to remember what you did now when you come back to change something 3 months from now...).

Welcome to the Treehouse Community

Looking to learn something new?

Björn Norén

Björn Norén

Good or bad oop-practise?

2 Answers

Semen Zadorozhnyi

Semen Zadorozhnyi

J.D. Sandifer

J.D. Sandifer