"if" for every item in inventory, looks ugly

Question

I am learning java right now and for some practice I am programming an inventory for a little game. You have 3 slots and in each you can put an item.

public boolean useItem(int place){
    //out of array request?
    if (place > items.length) return false;
    //empty inventory place?
    if (items[place] == 0) return false;

    if (items[place] == 1){
        //eat berrys
        items[place] = 0;
        return true;
    }
    if (items[place] == 2){
        //shoot arrow
        items[place] = 0;
        return true;
    }
    //item not there
    return false;
}

It works, but it feels bad to write down more and more "if" for every single item. Does anyone know a better solution?

Answer 1

Item item = items[place]; // by default you can have an EmptyItem that does nothing in the inventory when the place is empty

now let's imagine you have something like

interface Item {
 void use();
}

class Berry implements Item{
  @Override
  public void use(){
     //eating a a barry
  }
}

class EmptyItem implements Item{
  @Override
  public void use(){
     //do nothing
  }
}

then you would implement the logic in the method inside the use

item.use();

I would suggest you to create your inventory as bit more complex object instead of an array (the complex object can be composed itself using an array)

Imagine to have an inventory that can be created this way (passing its size as a parameter):

class Inventory{

    private Item[] items;

    public Inventory(int size){
        items = new Item[size];
        // references are null by default, just making this explicit for op
        for(int i = 0; i<size; i++){
           items[i] = null;
        }
    }

    public void addItem(int space, Item item){
       // check for input params
       // check array boundaries
       // check for item in the selected space
       if(items[space] == null){
           items[space] = item;
       }else{
          // space not empty!
       }
    }

    // I'd like more the approach of using get item -> item.use() :)
    // but this is a bit more inline with op approach
    public void useItem(int space){
       // check array boundaries
       Item item = items[space];
       if(item != null){
           item.use();
       }
    }    

    // keeping the null management inside the class itself
    public Item getItem(int space){
       // check array boundaries           
       Item item = items[space];
       if(item == null){
         item = new EmptyItem();
       }
       return item;
    }     
}

Just imagine what you are modeling to be real life objects, imagine how they intefact one another, and try to model their properties and behaviour accordingly :)

Answer 2

An alternative to an if else tree is the switch statement.

switch(items[place]) {
    case 0:
        return false;
    case 1:
        items[place] = 0;
        return true;
    case 2:
        items[place] = 1;
        return true;
    default:
        return false;
}

DEMO

Answer 3

I'd say use a custom class for items, something like:

class InventoryItems {
    private HashMap<Integer, Integer> items = new HashMap<Integer, Integer>();

    public InventoryItems(final HashMap<Integer, Integer> items) {
        this.items = items;
    }

    public boolean hasItem(final Integer index) {
        return items.containsKey(index) && items.get(index) > 0;
    }

    public boolean useItem(final Integer index) {
        if (!hasItem(index)) {
            return false;
        }

        items.put(index, 0);
        return true; 
    }
}

What you're doing is not wrong just makes it really hard to mentain in the long run. This is leaking all of the domain model logic outside and thus you have complex functional code that needs to be maintained and aware of the state of the application at any point of time.

Though for testing and learnign purposes I think as other people have proposed use a switch or if check for all possible items in the list.

Answer 4

This can be handled by a switch case. That would look like this:

public boolean useItem(int place){
    //out of array request?
    if (place > items.length) return false;
    switch (items[place) {
        case 0: //empty
        default:
        return false;
        case 1: //eat berrys
            items[place] = 0;
        return true;
        case 2: //shoot arrow
            items[place] = 0
        return true;
    }
    //item not there
    return false;
}