Console-based game in Java to demonstrate Solid principles and abstractions

Question

I've written a console-based demo while learning about the Solid principles. Along the way, I've become exceedingly paranoid about whether I'm writing good code or not.

Here is the code I have so far:

public class Entity {  //Base template for all game entities...

private String name;

public Entity(String name)
{
    this.name = name;
    displayEntityName();
}

public String getName()
{
    return name;
}

public void setName(String newName)
{
    name = newName;
}

private void displayEntityName()
{
    System.out.println(name + " entity created...");
}



public class Player extends Entity {

private String[] inventory; //Player will have an inventory.
private int itemsInInventory;
private static final Random RAND = new Random();

public Player(String name) {
    super(name);
    inventory = new String[25]; //Default inventory size.
}

public void attack(Enemy enemy) //Player can attack all enemy types.
{
    System.out.println(super.getName() + " attacked the enemy...");
}

public void findLegendaryWeapon()
{
    inventory[0] = WeaponConstants.LEGENDARY_WEAPONS
    [RAND.nextInt(WeaponConstants.LEGENDARY_WEAPONS.length)];
    itemsInInventory++;
}

public void equiptWeapon()
{
    System.out.println(super.getName() + " equipted a weapon...");
}

public void searchChest()
{
    System.out.println(super.getName() + " searched the chest and found nothing...");
}


}

public enum WeaponConstants{ //Legendary weapon constants.

BOW_OF_THE_MAJOR_ARCHER(), BLOOD_LETTER,
DAWNFANG, FROZEN_TOUCH;

public static final String[] LEGENDARY_WEAPONS = {DAWNFANG.name(), 
        BLOOD_LETTER.name(), FROZEN_TOUCH.name(), BOW_OF_THE_MAJOR_ARCHER.name()};

}


public interface Enemy { //Enemy interface...
    void attack(Player player);
}


public class Ghoul extends Entity implements Enemy { //An example of a standard enemy class.

    public Ghoul(String name) {
        super(name);
    }

    @Override
    public void attack(Player player) {
        System.out.println(super.getName() + " attacked " + player.getName());
    }
}

As mentioned above, I mainly want to know if I'm violating any of the Solid principles. My main concern is with my player class. I have quite a few methods in the class that do different types of things but they are all related to the player. Is this in any way violating the SRP principle?

Also, I wanted to know if my abstractions are correct and well-designed. Does this represent good coding style when used to develop more serious apps? I only have one year of experience, so I'm trying to solidify good practices early on. All input would be appreciated.

Answer 1

thanks for sharing your code

I know that this isn't a huge amount of code, but I'm going to look at it assuming that we will want to be able to add additional features and expand upon the code that's already here.

Redundant Comments

comments should say why you're doing something, not what you're doing. This line

private String[] inventory; //Player will have an inventory.

is an example of a particularly bad offender. If I have a Player class, and it has an inventory member variable. We know that a Player will have an inventory, you don't need to write a comment saying what the code is already telling us.

Let's take a look at your Entity class. If we look in the constructor, we can see that the constructor is causing some side effects. In this case, it's printing to the console. In general, when I construct an object, I assume there will be no side effects or other unexpected behaviour. It's perfectly reasonable to provide a method to do this (in your case displayEntityName) but I would not include this in the constructor.

Consider making your classes immutable where possible

Unless you need to provide getter and setter methods for an instance variable, you should just leave them out entirely and make the variable final. I think here name is a good candidate, a getter seems reasonable, but I don't think you'd ever really need a setter. In a standard RPG or game, the player or enemy names don't change. They can be marked final.

private final String name;

see this question https://stackoverflow.com/questions/565095/are-getters-and-setters-poor-design-contradictory-advice-seen

have a look at the arguments for and against using getters and setters. I won't also recommend reading up on the benefits (and drawbacks) of writing immutable classes.

Avoid using Strings to represent more complex Objects

You're currently representing your inventory as an array of Strings. I would argue that an Item is far more than just a String. I would rather see an Item class. This could be a super class or interface that all items in your game could derive from / implement.

Some class that come to mind could be, Weapon, Armour, Accessory etc. If all of these things are represented as a single String, you're restricting what you can do. Additionally, I would not use an array for this situation. How about an Inventory class.

public class Inventory {
    private final List<Item> items = new ArrayList<>();

    public void addItem(Item item){ // can be weapon, armour etc.
        items.add(item);
    }

    public int numItems(){
        return items.size();
    }
    ...
}

Now, this Inventory class has the single responsibility of representing and maintaining this list of items.

and your Player class can just have an Inventory

private final Inventory inventory;

Or even just put it in the Entity class. This way an Enemy can have an inventory too.

Notice how I used a List instead of an array. This can be resized and already has a list.size() method to tell you how many elements are in it. No need to keep track of that stuff yourself!

This Inventory can now become a lot more useful than just an array of Strings.

Your current Player class has two responsibilities that jump out. One is providing Player behaviour, but it's also maintaining the inventory by keeping track of it's size. Now the Inventory class is in charge of this.

This method signature

public void searchChest()

At the moment this method simply prints that the Player is searching a chest, why not provide the option to search any Chest object.

public void searchChest(Chest chest)

and a Chest could look like

public class Chest {
    private final List<Item> contents;

    ...

    public List<Item> open(){
        return new ArrayList<>(contents);
    }
}

Then in the searchChest method, the Items from the Chest can be added to the Inventory.

Similarly to the Chest method, this method

public void equiptWeapon()

firstly, there's a typo here. It should be

public void equipWeapon()

Secondly, why not be able to provide a weapon

public void equipWeapon(Weapon weapon)

At the moment in your Enemy interface you have this method.

void attack(Player player);

What if you want to have Enemies fight each other, different factions etc. There's no need to limit the potential targets, how about

void attack(Entity entity);

Some final points to consider.

When you attack another entity, you could get the currently equipped Weapon and the other entity's currently equipped Armour and do some simple math based on their Stats

instead of a displayEntityName method, you could just override toString and then print the Entity directly in the calling code.

Before you start coding anything, think about what Objects could be involved in your System/Game in real world terms, then start thinking about how these could be implemented as Objects in your code.

Instead of WeaponConstants, why not just make a Collection of Weapon objects.

Hopefully this review was useful for you.

Keep it up!

Answer 2

private void displayEntityName()

As this method is in the Entity class, "Entity" as part of the method name is superfluous.

---

Player.public void attack(Enemy enemy)
Enemy.void attack(Player player);

I predict duplicate code and frustrating type clashes as 'Player', 'Enemy', 'Goal' objects interact, desperate attempts at type-casting resulting in unnecessary classes and interfaces, etc.

Could you say 'Player', 'Enemy', 'Goal' are all combatants?

---

@Override
public void attack(Player player)

Overriding an interface method doesn't seem to make sense. Perhaps I'm missing something?

---

Inconsistancies:

Player subclass directly implements attack() method but Ghoul gets attack() via an interface. Yet all of these inherit Entity. Why can't Entity have an abstract or virtual method maybe with some default behavior? Or why can't all of these guys implement the Enemy interface?

The idea of an enemy is one sided. I say this because only one class implements that interface. Yet a player is a ghoul's enemy and a ghoul is a player's enemy; but the current design does not capture this commutative property. I'm not saying the answer is everyone implements the Enemy interface.

This lopsided enemy concept also manifests by the fact there are attack methods that take different types.

players have weapon methods, but ghouls do not. Another thing that should be consistent and expressed in some kind of common design construct.

Cohesion:

In terms of fighting each other these objects are all the same thing conceptually, let's say "combatants". This idea should consolidate the differently-typed attack methods and interfaces (and more to come I imagine). With a more abstract type, any combatant can fight any other combatant. Then it's other properties, methods, inheritance, etc. that differentiates a player, enemy, ghoul, etc.