2
\$\begingroup\$

I made a small script for Unity3D, meant to be use as GameComponent for GameObjects. It is supposed to represent values that have a max value, and an actual value. In games that would be the case, for instance, for the Health Points, the Stamina, Magic Points and so on. I would love to hear your feedback.

/////////////
//
// This is meant for things like Stamina, or HP. You have a max
// value, and an actual value.
//
// It inherits from MonoBehaviour because it is meant to be used
// as a Game Object Component
//
/////////////

using UnityEngine;
using UnityEngine.UI;

public class Range : MonoBehaviour {

    private int val, max;

    public Slider slider;

    // Is/Has/Can

    public bool IsDepleted() { return val == 0;    }
    public bool IsFull()     { return val == max; }

    public bool HasAtLeast(int _val) { return this.val >= _val; }

    // Actions

    public void Fill()    { val = max; }
    public void Deplete() { val = 0;   }

    public void Increase(int _val) { this.SetValue(this.val + _val); }
    public void Decrease(int _val) { this.SetValue(this.val - _val); }

    // Getters

    public int GetValue() { return this.val; }
    public int GetMax()   { return this.max; }

    // Setters

    public void SetValue(int amount) {
        if (amount > this.max) this.Fill();
        else if (amount < 0) this.Deplete();
        else this.val = amount;

        UpdateSlider();
    }

    public void SetMax(int amount) {
        this.max = amount;
        if (this.val > this.max) this.Fill();

        UpdateSlider();
    }

    private void UpdateSlider() {
        if (slider) {
            slider.maxValue = this.max;
            slider.value = val;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ I can't judge your code idioms because I don't know unity3D, but I love to give feedback for sake of fun. If I was contributing to your code, I would add "public bool HasLessThan(int _val) {return this.val <= _val;}" at it/has/can section so you can trigger events when stamina has gone below certain threshold or something. \$\endgroup\$ – user1235831 May 21 '16 at 14:34
  • \$\begingroup\$ There is one thing I have to ask though; could slider.maxValue be set within a constructor? Again, I don't know C# or unit3d idioms so I have to ask. Setting it on every update seems redundant to me. \$\endgroup\$ – user1235831 May 21 '16 at 14:43
  • \$\begingroup\$ HasLessThanis actually just the opposite of HasAtLeast, no? Is it the same as negating that method. About the Constructor, i didn't add it, because the class was not meanth to be used directly, but for inheritance. But it is not a bad idea. \$\endgroup\$ – Enrique Moreno Tent May 21 '16 at 15:33
  • \$\begingroup\$ You are right, HasLessThan is supposed to be opposite of HasAtLeast and you should be able to just negate it. I accidentally introduced a bug with the equal sign in the HasAtLeast function... I guess my commit was... rejected.. (•_•) / ( •_•)>⌐■-■ / (⌐■_■) \$\endgroup\$ – user1235831 May 21 '16 at 16:10
  • \$\begingroup\$ Appreciated in any case :) \$\endgroup\$ – Enrique Moreno Tent May 21 '16 at 16:18
2
\$\begingroup\$

Problematic Fill and Deplete are public, but don't call UpdateSlider, whereas Increment/Decrement do.

SetMax should reject values < 0 (0 could be allowed, for value forced to 0)

Design Style There is a function in the Unity Mathf static class that does bounds checking Mathf.Clamp

C# has properties, while you can have the pair GetX/SetX they are preferred.

public int Value { 
    get { return val; } 
    set { val = Mathf.Clamp( value, 0, Max ); 
        UpdateSlider(); } }
public int Max { 
    get { return max; } 
    set { if ( value < 0 ) throw new ArgumentOutOfRangeException ("Max"); 
        max = value; } }

Consider Increment and Decrement could be replaced by overriding + and -, as += and -= will use such overloads.

\$\endgroup\$
  • \$\begingroup\$ Those suggestions are wonderful! Could you give me an example of how would overloading the +operator look like? I have been trying for a while and I am a little confused about the syntax \$\endgroup\$ – Enrique Moreno Tent May 21 '16 at 15:56
  • \$\begingroup\$ Also please keep in mind that I would like Range to be inheritable, so the child classes should also be able to use the operator overload. \$\endgroup\$ – Enrique Moreno Tent May 21 '16 at 16:04
  • \$\begingroup\$ Overloading operators is basically incompatible with inheritance. Why do you want to inherit Range? \$\endgroup\$ – Caleth May 21 '16 at 16:31
  • \$\begingroup\$ Well, I wanted HPand Stamina` to work like Range, but with their own particularities. Do you think I will have to implement it directly on each Child Class, even if they do the same? \$\endgroup\$ – Enrique Moreno Tent May 21 '16 at 16:31
  • \$\begingroup\$ I think you are using inheritance too much. I don't think HP needs to be a class \$\endgroup\$ – Caleth May 21 '16 at 16:42

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.