Implementation of a string-based loot table

I wanted to create a loot table mechanic for any future game projects, and came up with what I am about to show you guys. I was wondering if someone could give it a glance and see if there could be any errors, or if it could be improved in any way.

```
using System;
using System.Collections.Generic;

namespace NovaEngineFramework.Framework.Probability
{
///
/// A String-based loot table.
/// The table is based upon a pie-chart design. In other words,
/// each item is added with a desired rarity; that rarity counts
/// towards a total maximum of 100. Example:
/// Item1 = 10
/// Item2 = 40
/// Item3 = 50
/// Total = 100.
/// Item1 is the rarest item in the table, while Item3
/// is the most common.
///
public class StringBasedLootTable
{
private readonly List _loot;
private readonly Dictionary _cachedItems;
private int _remainingStorage = 100;
private readonly Random _rngInstance;

public StringBasedLootTable()
{
_cachedItems = new Dictionary();
_loot = new List();
_rngInstance = new Random();
}

///
/// Adds an item to the Loot Table with it's own specified rarity.
/// An error may be thrown if the storage capacity(maximum 100) is
/// exceeded by a previously added item.
///
/// How rare the item will be.
/// The string name representation of the item to be added.
/// Thrown if the capacity has already been exceeded, or is about to be exceeded.
public void Store( int rarity , string itemName )
{
int test = ( _remainingStorage - rarity );
if( test
/// Shows the remaining storage of this loot table.
///
///
public int CheckRemainingStorage()
{
return _remainingStorage;
}
///
/// Rebuilds the loot table. An error will be thrown if

If you're going to build a game on this stuff, you better start coding object oriented. Good OO coding naturally manages complexity. Without coherent classes, proper encapsulation, etc. adding game complexity will turn your code base to mush.

public class Loot {
    public string Name {get; protected set;}
    public int Rarity { get; protected set;};

    public Loot (string theName, int theRarity) {
        Name = theName ?? string.Empty;
        Rarity = theRarity;
    }

    public override string ToString() {
        return string.Format("{0} : {1}", Name, Rarity);
    }

    // if your loot table can have only one of each kind of loot
    // then perhaps override Equals (and GetHashCode)
    // The fact that your Dictionary uses the name as the key 
    // tells me this is the case.

    public override bool Equals(object other){
        if(other == null) return false;

        Loot theOther = other as Loot;
        if(theOther == null) return false;

        return this.Name == theOther.Name;
    }
}

StringBasedLootTable

Strongly typed Dictionary List

private readonly List _cachedItems;

Store() has a bug - We don't want duplicates

public void Store( Loot theLoot ) {
   if(! _cachedItems.Contains( theLoot )  // made possible by Equals() override above.
        _cachedItems.Add(theLoot);
}

Implement an Equals(), could be handy

public bool Equals (Loot left, Loot right) {
    // null is never equal to null
    if (left == null || right == null)
        return false;

    return left.Equals(right);  // brought to you by: Loot.Equals() override!
}

Encapsulate the "remainingStorage"

protected int MaxStorage {get { return 100; }} // yeah, maybe a constant instead.
protected int CurrentStorage {
    get { return _cachedItems.Sum( x => x.Rarity ); }
}

public int RemainingStorage {get {return MaxStorage - CurrentStorage; } }

public bool CanStore(Loot thisLoot) {
    return thisLoot.Rarity + CurrentStorage <= MaxStorage;

       // duh, radarbob. Could be simpler still. Good ol' OO.
    return thisLoot.Rarity <= RemainingStorage;
}

• I don't understand why there is both _loot and _cachedItems. In any case I expect they are not both needed.

• CheckRemainingStorage() - delete this.

• If you must have Rebuild(), how in the world is the client code supposed to know when and why to call it? The table should be re-building itself when necessary.

• I like that the Dictionary List is encapsulated, rather than StringBasedLootTable inheriting from List. This controls exposure of all the list public methods and allows you to write methods in terms of a loot table - thus the class has the "look and feel" (API) of a "loot table".

• Good OO code tends to have short(er) methods. Good OO coding manages complexity. The CanStore(), Equals(), and CurrentStorage implementations demonstrate this.

StackExchange Code Review Q#91139, answer score: 7