2
\$\begingroup\$

I would prefer if more experienced users could give pointers on how I can optimize and think better when writing code.

If you are unfamiliar with Unity3d, ignore the use of UnityEngine, the heritage from MonoBehaviour as well as the Debug.Log(), Debug.LogWarning(), and Debug.LogError().

Inventory Script (RPG) in C#

This is an updated version of the inventory code from there, nothing in that question is needed for this.

using UnityEngine;

public class Party : MonoBehaviour {

    //private MemberCharacter[] members;
    private Item[] inventory;

    private int _currency;

    public const int INVENTORY_SIZE = 50; // will be changed later to 50*party members

    public Party () {
        inventory = new Item[INVENTORY_SIZE];
    }

    // add q items
    // first stack in existing items
    // then make new slot
    // if there is no place it returns the overflow
    public int AddItem (Item item, int q) {

        // stores items left
        int tRemain = q;

        // checks if the item already exist in inventory
        // adding to its stack      
        int idxFree = FirstFreeSlot();
        for (int i = 0; i < inventory.Length; ++i) {

            Item tItem = inventory[i];

            if (tItem == item && tItem.MaxQuantity > tItem.Quantity) {

                // gets MaxQuantity - Quantity - q (remaining stacks left)
                tRemain = inventory[i].Add(q);

                // no stacks of item left
                if(tRemain == 0) {
                    Debug.Log ("All  " + q + "x " + item.Name + " added.");
                    return 0;
                }
            }
        }

        // is there a free slot?
        while (idxFree >= 0) {

            // create new items and remove from tRemain
            inventory[idxFree] = item;
            tRemain = inventory[idxFree].Add (tRemain);

            // any remaining items?
            if(tRemain == 0) {
                Debug.Log ("All  " + q + "x " + item.Name + " added.");
                return 0;
            }

            // checking for another slot
            idxFree = FirstFreeSlot();
        }

        // no free slots
        // returns items in stack left
        Debug.LogWarning ("Inventory full! " + q + "x " + item.Name + " left.");
        return tRemain;
    }

    // swaps items in two slots
    public void Move (int src, int dst) {
        Item tItem = inventory[dst];
        inventory [dst] = inventory [src];
        inventory [src] = tItem;
    }

    // remove q items at i
    // check when calling function to make sure q is greater than or equal to ItemSlot[i].quanity
    public void RemoveItem (int i, int q) {

        // removed too many!
        if(inventory[i].Remove(q) == false){
            Debug.LogError ("Quantity at index " + i + " is " + inventory[i].Quantity + "!");
            DeleteItem (i);
        }

        // removed all? delete item at i
        if (inventory [i].Quantity == 0) {
            Debug.Log ("Deleted " + inventory[i].Name);
            DeleteItem (i);
        }       
    }

    // item at i has a q of 0, delete item
    private void DeleteItem (int i) {
        inventory [i] = null;
    }


    // finds a free slot, returns -1 when no slot is present
    private int FirstFreeSlot(){
        for(int i = 0; i < inventory.Length; ++i){
            if(inventory[i] == null)
                return i;
        }
        return -1;
    }

    // checks if q items exists in inventory
    // returns the number of missing items
    // 0 item and quantity are met
    // -1 item is not met
    public int FindItem (Item item, int q) {
        return -1;
    }

#region Setters and Getters
    public int Currency {
        get {return _currency;}
        set {_currency = value;}
    }
#endregion
}

Item Class:

using UnityEngine;

public class Item : MonoBehaviour {

    private string _name;
    private string _desc;

    private int _quantity;
    private int _maxQuantity;

    private int _category;
    private bool _dropable;
    private int _price;

    public Item () {
        _name = string.Empty;
        _desc = string.Empty;

        _quantity = 0;
        _maxQuantity = 0;

        _category = 0;
        _dropable = false;
        _price = 0;
    }

    // returns the quatity left to add
    // returns 0 when there are no more items
    public int Add(int q){

        // does q fit into _quantity?
        if (q + _quantity <= _maxQuantity) {
            Debug.Log ("Added to existing item in one stack " + q + "x " + _name);
            _quantity += q;
            return 0;
        }

        // need more slots than 1 add as many as possible
        // returns what is left of q
        int tRemain = _maxQuantity - _quantity;
        Debug.Log("Added " + tRemain + "x " + _name + " - " + q + " left.");
        q -= tRemain;
        _quantity = _maxQuantity;
        return q;
    }

    // remove items from quantity
    // returns true if _quantity > 0 after execution
    public bool Remove (int q){
        if(_quantity >= q){
            _quantity -= q;
            Debug.Log ("Removed " + q + "x " + _name);
            return true;
        }

        // can't remove this many!
        return false;
    }

#region Setters and Getters
    public string Name {
        get {return _name;}
        set {_name = value;}
    }

    public string Desc {
        get {return _desc;}
        set {_desc = value;}
    }

    public int Quantity {
        get {return _quantity;}
        set {_quantity = value;}
    }

    public int MaxQuantity {
        get {return _maxQuantity;}
        set {_maxQuantity = value;}
    }

    public int Category {
        get {return _category;}
        set {_category = value;}
    }

    public bool Dropable {
        get {return _dropable;}
        set {_dropable = value;}
    }

    public int Price {
        get {return _price;}
        set {_price = value;}
    }
#endregion
}


// enum used to identify categories by name
public enum ItemCategory {
    Miscellaneous,
    Quest,
    Consumable,
    Weapon,
    Armor,
    Gem,
    Resource,
};

I know you aren't supposed to have two questions in one question but I take it I should probably make a singleton Inventory class to mange the inventory now?

\$\endgroup\$
2
\$\begingroup\$

Here is a bug:

int tRemain = q;

for (...)
    if (...)
        tRemain = inventory[i].Add(q);

I think the above should be tRemain = inventory[i].Add(tRemain);.

I don't see why you have tRemain as an extra/new variable, instead of just using q.

        q = inventory[i].Add(q);

Here is how I would code the AddItems method:

public int AddItem (Item item, int q) {

    // no because it's confusing to have more than one varable
    //int tRemain = q;

    // no because we're about the iterate the array anyway
    //int idxFree = FirstFreeSlot();

    for (int i = 0; i < inventory.Length; ++i) {

        Item tItem = inventory[i];

        // no need to check tItem.MaxQuantity because it's checked inside Item.Add
        //if (tItem == item && tItem.MaxQuantity > tItem.Quantity) {

        if (tItem == item) {

            // gets MaxQuantity - Quantity - q (remaining stacks left)
            q = inventory[i].Add(q);

            // no stacks of item left
            if(q == 0) {
                Debug.Log ("All  " + q + "x " + item.Name + " added.");
                return 0;
            }
        }

        if (tItem == null) {
            // code which used to be after the for loop

            // no because we're already in a for loop and at the right index
            //while (idxFree >= 0)
            inventory[i] = item;
            q = inventory[i].Add(q);

            // any remaining items?
            if(q == 0) {
                Debug.Log ("All  " + q + "x " + item.Name + " added.");
                return 0;
            }

            // no because we're already in a for loop and at the right index
            //idxFree = FirstFreeSlot();
        }
    }

    // no free slots
    // returns items in stack left
    Debug.LogWarning ("Inventory full! " + q + "x " + item.Name + " left.");
    return q;
}

I think there's another bug in your architecture: which is that when an Item instance is full then you put the same Item instance into the next slot. If the Item is full in one slot, then the same Item instance is full in the next slot.

With your existing architecture you can't store an Item in more than one slot, if you have more than maxQuantity of that item, which you seem to be trying to do.

\$\endgroup\$
  • \$\begingroup\$ I use it for Debug.Log() currently. I don't see the supposed error. I don't get any warnings of any sort. \$\endgroup\$ – Emz Jan 17 '14 at 22:26
  • \$\begingroup\$ For example if q is 3 to begin with, and you find an slot that accepts 2, then tRemain is 1. Then the next time through the for loop you might find another partially-empty slot: if so then you add q (i.e. 3) again, instead of adding tRemain (i.e. 1). You will go through the for loop again because you don't break; when you find the first match. There may be more than one matching non-filled slot because your RemoveItem method allows you to remove quantities from any slot. \$\endgroup\$ – ChrisW Jan 17 '14 at 22:40
  • \$\begingroup\$ Oh yes. That forces me to use another variable for the debugging. Silly me, I will update the post with q for now. \$\endgroup\$ – Emz Jan 17 '14 at 22:42
  • \$\begingroup\$ @Emz I think after that fix you can follow up your progress for a while now. I wouldn't make any major changes to your code. Show a bit effort and only post other question (if you want to) after making some progress. Don't forget to run and debug your code before posting a code review or ask a question. \$\endgroup\$ – Bruno Costa Jan 17 '14 at 23:17
  • \$\begingroup\$ @Emz I edited my answer. \$\endgroup\$ – ChrisW Jan 17 '14 at 23:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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