Basic console game, with combatants and inventory items

Question

I've tried to improve upon my last design of a basic console game, practicing OOP and treating more things as objects. So here is my revision. Is this good software design or is it really bad?

I know no one is perfect and people will want to do things differently than me, but it's nice to get some feedback on my style of coding.

Obviously this isn't complete and it's for practice, so there's a lot of code that I haven't bothered to implement yet, like the enemy behaviors, etc.

Here are some code samples:

public class Player extends Combatant {

    private int level; //Some fields I'll do stuff with later on.
    private int xp;
    private final Inventory inventory; //Only the player needs an inventory for now. 


    public Player()
    {
        inventory = new Inventory();
    }

    public void searchChest(Chest chest)
    {
        List<Item> items = chest.open();
        System.out.println(toString() + 
        " searched the chest and found: " + items + "\n");
        storeItemsInInventory(items);
    }

    public void storeItemsInInventory(List<Item> items)
    {
      inventory.addItems(items);
    }

    public void inspectInventoryItem(String value)
    {
     for(int i=0; i<inventory.numItems(); i++)
     {
         Item item = inventory.getInventoryItems().get(i);
         if(value.equals(item.toString()))
         {
             item.displayAllAttributes();
         }
     }
    }

    public void dropAllInventoryItems()
    {
      inventory.getInventoryItems().clear();
      System.out.println(this.toString() + " dropped all items from their inventory.");
    }
}

---

public class Inventory {

    private final List<Item> items = new ArrayList<>();

    public void addItem(Item item)
    {
        items.add(item);
    }

    public void addItems(List<Item> itemsToAdd)
    {
        for(Item i: itemsToAdd)
        {
            items.add(i);
            System.out.println("Stored " + i + " in inventory.\n");
        }
    }

    public List<Item> getInventoryItems()
    {
        return items;
    }

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

---

public abstract class Item {  

    private final String rarity;
    private int value;

    public static final Random RAND = new Random();
    public static final String[] RARITY_CONSTS = {"Shimmering", "Refined", "Flawed"};
    public static final List<Item> ITEM_CONSTS = Arrays.asList(new Diamond(), new GoldenNecklace(), new Ruby());

    public Item(int value, String rarity) //Can either choose an implementation of an item directly.
    {
        this.rarity = rarity;
        this.value = value;
        setValueAttributes();
    }

    public Item(int value) //Or the item can be assigned a random rarity upon creation.
    {
        this(value, RARITY_CONSTS
        [RAND.nextInt(RARITY_CONSTS.length)]);
        this.value = value;
    }

    private void setValueAttributes() //Determines the value boost based on the item's rarity.
    {
        switch(rarity)
        {
        case "Shimmering":
            value += 500;
                break;
        case "Flawed":
            value += 50;
                break;
        case "Refined":
            value += 250;
                break;
        }
    }


    public int getValue()
    {
        return value;
    }

    public String getRarity()
    {
        return rarity;
    }

    public String toString()
    {
        return this.getClass().getSimpleName();
    }

    public void displayValue()
    {
        System.out.println(toString() + "'s value is: " + value);
    }

    public void displayRarity()
    {
        System.out.println(toString() + "'s rarity is: " + rarity);
    }

    public void displayAllAttributes()
    {
        displayValue();
        displayRarity();
    }
}

---

public class Ruby extends Item{

    public Ruby() {
        super(300); //Normal ruby has a base value of 300.
    } 
              //More to implement in here later.

}

---

public class Ghoul extends Combatant {

    //Will fill functionality later.



}

Answer 1

Basically, its good. Here are my (small) comments

1) in Inventory.addItems() you can replace the loop with items.addAll(itemsToAdd)

2) I would make RARITY_CONSTS an enum. This has two benefits:

• checking equality vs constants like you do in setValueAttributes(), you could make a typo in rarity name and never find out until run time . when you use enum, the compiler checks this for you.

• in setValueAttributes() you match a rarity with added value. this is better put in the enum:

  public static enum RARITY
  {
      Shimmering(500),
      Refined(250),
      Flawed(50);
  
      private int addedValue;
  
      RARITY(int addedValue)
      {
          this.addedValue = addedValue;
      }
  
      public int addedValue()
      {
          return addedValue;
      }
  }
  

• now setValueAttributes() looks like this: value += rarity.addedValue(); isn't that nice ? :)

• with an enum, you can add more attributes and behavior (=methods) to rarity