Lvl 1 upgradeable attributes

Question

I've built the groundwork for an attributes (as in Strength, Intelligence, not as in DebuggerHidden, TestMethod) framework for my game. Each attribute can be upgraded and downgraded individually as well as part of a bulk change (such as for upgrades that have side effects, or leveling up).

I'm looking to get a thorough critique of the groundwork before I go expanding on shaky foundations.

UpgradeableStat and UpgradeableStat<T>

These are the absolute bases of the attributes system. UpgradeableStat only really exists so as to allow me to hold a collection of UpgradeableStat instances with different T types, all attributes will really extend from UpgradeableStat<T>

public abstract class UpgradeableStat
{
    public abstract void Upgrade();

    public abstract void Downgrade();
}

public abstract class UpgradeableStat<T> : UpgradeableStat
{
    public abstract T Value
    {
        get;
    }
}

OneShotStat

These attributes will be applied once, until it's been removed (whereby it's applied again). Think of it as a door, you can't open it again unless it's been closed.

/// <summary>
/// A statistic that is upgraded only once until it is next removed.
/// </summary>
public class OneShotStat<T> : UpgradeableStat<T>
{
    /// <summary>
    /// Gets the value.
    /// </summary>
    /// <value>
    /// The value.
    /// </value>
    public override T Value
    {
        get
        {
            return Upgraded ? UpgradedValue : DowngradedValue;
        }
    }

    /// <summary>
    /// Gets or sets the upgraded value.
    /// </summary>
    /// <value>
    /// The upgraded value.
    /// </value>
    public T UpgradedValue
    {
        get;
        set;
    }


    /// <summary>
    /// Gets or sets the downgraded value.
    /// </summary>
    /// <value>
    /// The downgraded value.
    /// </value>
    public T DowngradedValue
    {
        get;
        set;
    }

    /// <summary>
    /// Gets a value indicating whether this <see cref="OneShotStat"/> is upgraded.
    /// </summary>
    /// <value>
    ///   <c>true</c> if upgraded; otherwise, <c>false</c>.
    /// </value>
    public bool Upgraded
    {
        get;
        private set;
    }


    /// <summary>
    /// Upgrades this instance if it is not already upgraded.
    /// </summary>
    public override void Upgrade()
    {
        if (!Upgraded)
        {
            Upgraded = true;
        }
    }

    /// <summary>
    /// Downgrades this instance if it is not already downgraded.
    /// </summary>
    public override void Downgrade()
    {
        if (Upgraded)
        {
            Upgraded = false;
        }
    }
}

LevelThresholdOneShotStat

This is a fairly common attribute type. The attribute is set to the level up value only when the attribute's level is raised above a certain threshold. This is good for attributes that have a super form, for example.

/// <summary>
/// A OneShotStat that is applied when the stat breaches a level threshold and is removed when it sinks below it.
/// </summary>
public class LevelThresholdOneShotStat<T> : OneShotStat<T>
{
    /// <summary>
    /// The level
    /// </summary>
    private int level;

    /// <summary>
    /// Gets the level.
    /// </summary>
    /// <value>
    /// The level.
    /// </value>
    public int Level
    {
        get
        {
            return level;
        }
        private set
        {
            level = Math.Max(MinLevel, Math.Min(level, MaxLevel));

            if (Level >= Threshold)
            {
                base.Upgrade();
            }
            else
            {
                base.Downgrade();
            }
        }
    }

    /// <summary>
    /// The threshold
    /// </summary>
    private int threshold;

    /// <summary>
    /// Gets or sets the threshold.
    /// </summary>
    /// <value>
    /// The threshold.
    /// </value>
    /// <exception cref="System.ArgumentOutOfRangeException">
    /// Threshold;Argument must not be greater than MaxLevel
    /// or
    /// Threshold;Argument must not be less than MinLevel
    /// </exception>
    public int Threshold
    {
        get
        {
            return threshold;
        }
        set
        {
            if (value > MaxLevel)
            {
                throw new ArgumentOutOfRangeException("Threshold", "Argument must not be greater than MaxLevel");
            }

            if (value < MinLevel)
            {
                throw new ArgumentOutOfRangeException("Threshold", "Argument must not be less than MinLevel");
            }

            threshold = value;

            Level = Level;
        }
    }

    /// <summary>
    /// The minimum level
    /// </summary>
    private int minLevel;

    /// <summary>
    /// Gets or sets the minimum level.
    /// </summary>
    /// <value>
    /// The minimum level.
    /// </value>
    /// <exception cref="System.ArgumentOutOfRangeException">MinLevel;Argument must not be greater than MaxLevel</exception>
    public int MinLevel
    {
        get
        {
            return minLevel;
        }

        set
        {
            if (value > MaxLevel)
            {
                throw new ArgumentOutOfRangeException("MinLevel", "Argument must not be greater than MaxLevel");
            }

            minLevel = value;

            if (Level < MinLevel)
            {
                Level = MinLevel;
            }
        }
    }

    /// <summary>
    /// The maximum level
    /// </summary>
    private int maxLevel;

    /// <summary>
    /// Gets or sets the maximum level.
    /// </summary>
    /// <value>
    /// The maximum level.
    /// </value>
    /// <exception cref="System.ArgumentOutOfRangeException">MaxLevel;Argument must not be less than MinLevel</exception>
    public int MaxLevel
    {
        get
        {
            return maxLevel;
        }
        set
        {
            if (value < MinLevel)
            {
                throw new ArgumentOutOfRangeException("MaxLevel", "Argument must not be less than MinLevel");
            }

            maxLevel = value;

            if (Level > MaxLevel)
            {
                Level = MaxLevel;
            }

        }
    }

    /// <summary>
    /// Upgrades this instance.
    /// </summary>
    public override void Upgrade()
    {
        Level++;
    }

    /// <summary>
    /// Downgrades this instance.
    /// </summary>
    public override void Downgrade()
    {
        Level--;
    }
}

InterpolatedUpgradeableStat

This is probably the most common type of attribute. It starts at one value and increases with each upgrade up to a maximum.

/// <summary>
/// Upgradeable stat whose value is set by interpolating between a minimum and maximum value based on the stat's level.
/// </summary>
/// <typeparam name="T">Type of internal value of the statistic</typeparam>
public class InterpolatedUpgradeableStat<T> : UpgradeableStat<T>
{
    /// <summary>
    /// Gets the value.
    /// </summary>
    /// <value>
    /// The value.
    /// </value>
    public override T Value
    {
        get
        {
            return interpolator.Interpolate(Min, Max, (float)Level / (float)upgradeSteps);
        }
    }

    /// <summary>
    /// The interpolator
    /// </summary>
    private IInterpolator<T> interpolator;

    /// <summary>
    /// Gets or sets the interpolator.
    /// </summary>
    /// <value>
    /// The interpolator.
    /// </value>
    /// <exception cref="System.ArgumentNullException">Interpolator</exception>
    public IInterpolator<T> Interpolator
    {
        get
        {
            return interpolator;
        }
        set
        {
            if (value == null)
            {
                throw new ArgumentNullException("Interpolator");
            }

            interpolator = value;
        }
    }

    /// <summary>
    /// The upgrade steps
    /// </summary>
    private int upgradeSteps;

    /// <summary>
    /// Gets or sets the upgrade steps.
    /// </summary>
    /// <value>
    /// The upgrade steps.
    /// </value>
    /// <exception cref="System.ArgumentOutOfRangeException">UpgradeSteps;Argument must be not be less than 0.</exception>
    public int UpgradeSteps
    {
        get
        {
            return upgradeSteps;
        }
        set
        {
            if (value < 0)
            {
                throw new ArgumentOutOfRangeException("UpgradeSteps", value, "Argument must be not be less than 0.");
            }

            upgradeSteps = value;
        }
    }

    /// <summary>
    /// The maximum
    /// </summary>
    private T max;

    /// <summary>
    /// Gets or sets the maximum.
    /// </summary>
    /// <value>
    /// The maximum.
    /// </value>
    /// <exception cref="System.ArgumentOutOfRangeException">UpgradeSteps;Argument must be not be less than Min.</exception>
    public T Max
    {
        get
        {
            return max;
        }
        set
        {
            if (value is IComparable && ((IComparable)value).CompareTo(Min) < 0)
            {
                throw new ArgumentOutOfRangeException("UpgradeSteps", value, "Argument must be not be less than Min.");
            }

            max = value;
        }
    }


    /// <summary>
    /// The minimum
    /// </summary>
    private T min;

    /// <summary>
    /// Gets or sets the minimum.
    /// </summary>
    /// <value>
    /// The minimum.
    /// </value>
    /// <exception cref="System.ArgumentOutOfRangeException">UpgradeSteps;Argument must be not be greater than Max.</exception>
    public T Min
    {
        get
        {
            return min;
        }
        set
        {
            if (value is IComparable && ((IComparable)value).CompareTo(Max) > 0)
            {
                throw new ArgumentOutOfRangeException("UpgradeSteps", value, "Argument must be not be greater than Max.");
            }

            min = value;
        }
    }

    /// <summary>
    /// Initializes a new instance of the <see cref="InterpolatedUpgradeableStat{T}"/> class.
    /// </summary>
    /// <param name="min">The minimum.</param>
    /// <param name="max">The maximum.</param>
    /// <param name="upgradeSteps">The upgrade steps.</param>
    /// <param name="interpolator">The interpolator.</param>
    public InterpolatedUpgradeableStat(T min, T max, int upgradeSteps, IInterpolator<T> interpolator)
    {
        this.Min = min;
        this.Max = max;
        this.UpgradeSteps = upgradeSteps;
        this.Interpolator = interpolator;
    }

    /// <summary>
    /// Gets the level.
    /// </summary>
    /// <value>
    /// The level.
    /// </value>
    public int Level
    {
        get;
        private set;
    }

    /// <summary>
    /// Upgrades this instance.
    /// </summary>
    public override void Upgrade()
    {
        //Level range is 0..upgradeSteps-1
        Level = Math.Min(upgradeSteps - 1, Level + 1);
    }

    /// <summary>
    /// Downgrades this instance.
    /// </summary>
    public override void Downgrade()
    {
        Level = Math.Max(0, Level - 1);
    }
}

Answer 1

You know, code wise there is not really much to do. The other comments covered the few ways to improve things.

I would though suggest a look at your goals architecturally.

Aside from the semantically different meaning of an upgradable vs downgradable attribute both being under the same umbrella being a problem, in theory I don't see why an upgradable attribute is a subset of attribute.

In theory any location you want to use an attribute it should also be swappable by an modified attribute right?

Also Upgrading and Downgrading are two specific use cases.at some stage you may want to freeze an attributes values, or make a combined attribute or have a modifier only available if a condition is met. All of that would require some level of a redesign.

If you instead looked at an attribute as a base component:

IAttribute<T>
{
  T Value{ get; }
}

and a modification as a completely different thing:

IModifier<T>
{
  T Modify(T item)

  bool IsActive {get;set;}
}

You can now create an upgradable attribute but have your consuming code agnostic of its base type.

Then, you have scope for some other changes:

ModifiedAttribute<T> : IAttribute<T>
{
    IList<IModifier<T>> Modifiers{ get; }

    bool UseModifiers{ get; }

    T Value 
    {
   // here you might return a base value, but if use modifiers is selected and there are any, loop through and apply them. 
    }
}

This same separation of concerns can now be applied directly to the modifiers. Picture adding a conditional modifier, that only applies if the player has certain buffs or attributes, such as

return (Player.Buffs.Contains("Poison Resistant"))

or a weighted Modifier, which is just a modifier with an Int value, now you can sort based on that value and apply the modifiers in order based on their weight value. still agnostic from the Attribute.

or whatever.... and conditionally apply these modifiers.

The possibilities are endless,

---

In short, when architecting things, try to distill to the lowest base type and establish what consuming code really needs to know, absolute minimum, to avail of it's functionality.

Any additional functionality can always be passed in through constructor arguments or public properties on an implementation.

Answer 2

Auto-properties are convenient, because they let you declare properties in a compact form. This defeats the purpose:

public T UpgradedValue
{
    get;
    set;
}

Should be a simple one-liner:

public T UpgradedValue { get; set; }

Same here:

public abstract T Value
{
    get;
}

Get-only abstract properties are easily eye-parsed as a one-liner, too:

public abstract T Value { get; }

@BrunoCosta already mentioned it, but I can't help mentioning it too:

/// <summary>
/// Gets the value.
/// </summary>
/// <value>
/// The value.
/// </value>

Ugh. XML comments like this bring nothing to the table and do nothing other than cluttering up the code. Remove them, be merciless :)

XML comments only make sense for public members. This one is useless, on top of being redundant:

/// <summary>
/// The level
/// </summary>
private int level;

Picture XML comments as documentation - you can export them and build an actual help file, or MSDN-like documentation for your API; if all they do is state the obvious, you'll have a very frustrating documentation! Make comments useful, or remove them.

Answer 3

Congratulations, You've done a pretty amazing job by correctly using Math.Max and Math.Min on your code, improving readability. So I wonder why you didn't do the same in MaxLevel and MinLevel of LevelThresholdOneShotStat:

public int MinLevel
{
    get
    {
        return minLevel;
    }

    set
    {
        if (value > MaxLevel)
        {
            throw new ArgumentOutOfRangeException("MinLevel", "Argument must not be greater than MaxLevel");
        }

        minLevel = value;
        Level = Math.Max(Level, value);
    }
}

public int MaxLevel
{
    get
    {
        return maxLevel;
    }
    set
    {
        if (value < MinLevel)
        {
            throw new ArgumentOutOfRangeException("MaxLevel", "Argument must not be less than MinLevel");
        }

        maxLevel = value;
        Level = Math.Min(Level, value);
    }
}

Comments can be convenient and everything but not in this scenario. The only thing that most of your comments do in this case is add more lines to the file and make it harder to focus on the code.

Edit:

You could also simplify the logic of the Upgrade and DownGrade methods in OneShotStat. you can simply assign Upgraded to true or to false respectivly:

public override void Upgrade()
{
    Upgraded = true;
}

public override void Downgrade()
{
    Upgraded = false;
}

That is just because making a comparison is at least as expensive as making an assignment. Although you may prefer the former implementation if it represents more logically how the object behaves.