4
\$\begingroup\$

Is there a way I could somehow have all this classes condensed into one, while keeping the code as clean as possible?

They are all doing essentially the same thing, just for the different number type. I don't know how to "generalize" TryParse() without using switch or if tree which doesn't seem very clean to me.

Base class:

public class TextBoxNum<T> : TextBox where T : IComparable<T>
{
    protected T min;
    protected T max;

    protected Value<T> value;

    public virtual void Update(object sender, EventArgs e)
    {

    }

    protected T Clamp(T input) 
    {
        if (input.CompareTo(max) > 0)
        {
            return max;
        }

        if (input.CompareTo(max) < 0)
        {
            return min;
        }

        return input;
    }

    public TextBoxNum(Value<T> value, T min, T max)
    {
        this.value = value;
        this.min = min;
        this.max = max;

        LostFocus += new EventHandler(Update);           
    }
}

Int variant:

public class TextBoxInt : TextBoxNum<int>
{
    public override void Update(object sender, EventArgs e)
    {
        int newValue;

        if (int.TryParse(Text, out newValue))
        {
            if (newValue < min || newValue > max)
            {
                newValue = Clamp(newValue);
            }

            value.Set(newValue);
            Text = value.Get().ToString();
        }

        else
        {
            Text = value.Get().ToString();
            Focus();
        }
    }

    public TextBoxInt(Value<int> value, int min, int max) : base(value, min, max)
    {

    }
}

UInt variant:

public class TextBoxUInt : TextBoxNum<uint>
{
    public override void Update(object sender, EventArgs e)
    {
        uint newValue;

        if (uint.TryParse(Text, out newValue))
        {
            if (newValue < min || newValue > max)
            {
                newValue = Clamp(newValue);
            }

            value.Set(newValue);
            Text = value.Get().ToString();
        }

        else
        {
            Text = value.Get().ToString();
            Focus();
        }
    }

    public TextBoxUInt(Value<uint> value, uint min, uint max) : base(value, min, max)
    {

    }
}

ULong variant:

public class TextBoxULong : TextBoxNum<ulong>
{
    public override void Update(object sender, EventArgs e)
    {
        ulong newValue;

        if (ulong.TryParse(Text, out newValue))
        {
            if (newValue < min || newValue > max)
            {
                newValue = Clamp(newValue);
            }

            value.Set(newValue);
            Text = value.Get().ToString();
        }

        else
        {
            Text = value.Get().ToString();
            Focus();
        }
    }

    public TextBoxULong(Value<ulong> value, ulong min, ulong max) : base(value, min, max)
    {

    }
}

Float variant:

public class TextBoxFloat : TextBoxNum<float>
{
    public override void Update(object sender, EventArgs e)
    {
        float newValue;

        if (float.TryParse(Text, out newValue))
        {
            if (newValue < min || newValue > max)
            {
                newValue = Clamp(newValue);
            }

            value.Set(newValue);
            Text = value.Get().ToString();
        }

        else
        {
            Text = value.Get().ToString();
            Focus();
        }
    }

    public TextBoxFloat(Value<float> value, float min, float max) : base(value, min, max)
    {

    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ If you enforce that T is of type IConvertable and you're willing to handle exceptions rather than the bool result from TryParse, you can use Convert.ChangeType. That said, why not just use a NumericUpDown control data bound to a value of the type you want? \$\endgroup\$ – Dan Lyons Sep 15 '16 at 18:12
3
\$\begingroup\$

class TextBoxNum

  • Abbreviation should be avoided when naming a class: Num -> Numeric
  • The descriptive adjective should also be in front of the noun. It is a numerical textbox, and not a textbox number: NumericTextBoxBase
  • TextBoxNum and Update should be marked as abstract, unless a generic way of updating the value is provided in TextBoxNum. And by changing it to a abstract class, we should append Base to the end of the class name : NumericTextBoxBase, and its concrete classes can be named as FloatTextBox, DoubleTextBox...

class Value<T>

Without the definition of this class, I can only make some guess about it. We can observe that the value is assigned by a parameter from the constructor. The class itself seems to serve no other purpose than holding the value via void Value<T>.Set(T) and T Value.Get(). I would suggest you to replace them by T or T?, depending on whether or not if you want to enforce valid value is required.


Misc

  • Avoid try-catch as control-structure much as possible, unless that is your last resort.
  • Your Clamp method is comparing the value against the same upper boundary, max, twice.

Code:

public class NumericTextBox<T> : TextBox where T : IComparable<T>
{
    protected T min;
    protected T max;
    protected T value;

    public NumericTextBox(T value, T min, T max)
    {
        this.min = min;
        this.max = max;

        // ensure the value is with in valid range
        this.value = Clamp(value);
        // actually you should implement a full property for value and update the Text property in the setter
        this.Text = value.ToString();

        LostFocus += new EventHandler(Update);
    }

    protected virtual void Update(object sender, EventArgs e)
    {
        var type = typeof(T);
        var tryParse = type.GetMethod("TryParse", new[]
        {
            typeof(string), type.MakeByRefType()
        });
        var args = new object[] { Text, null };
        var result = (bool)tryParse.Invoke(null, args);

        if (result)
        {
            value = Clamp((T)args[1]);
            Text = value.ToString();
        }
        else
        {
            Text = value.ToString();
            Focus();
        }
    }
    protected T Clamp(T input)
    {
        if (input.CompareTo(max) > 0)
        {
            return max;
        }

        if (input.CompareTo(min) < 0)
        {
            return min;
        }

        return input;
    }
}
\$\endgroup\$
  • \$\begingroup\$ I generally use try-catch for errors in the program main. I'm curious why I should avoid it? \$\endgroup\$ – pacmaninbw Sep 15 '16 at 20:24
  • \$\begingroup\$ @pacmaninbw You should avoid it as much as possible, as throwing is a very expensive operation. Try-catching in the main method is fine, as this is your last resort before the application crashes without any context and you want to avoid that. However, in this case, try-catch is simply used as a control structure when there is better way(TryParse) of handling it. \$\endgroup\$ – Xiaoy312 Sep 15 '16 at 20:40
  • \$\begingroup\$ value = Clamp((T)args[1]); should be value.set(Clamp((T)args[1]));. Also value.ToString() is value.Get().ToString(). I don't know what you mean about Value<T>. It's role is to be used as a wrapper for passing value types and it's an observable so I can track the wrapped value. I decided against Ref keyword since there is a lot of Value<T> objects and I pass them around a lot. This prevents oversights since it throws compile time error. What does I would suggest you to replace them by T or T?, depending on whether or not if you want to enforce valid value is required. this mean? \$\endgroup\$ – Karlovsky120 Sep 15 '16 at 21:24
  • 1
    \$\begingroup\$ sorry, but this design is horrible - how you can upvote a public event handler? ;-] \$\endgroup\$ – t3chb0t Sep 16 '16 at 7:19
  • \$\begingroup\$ @Karlovsky120 Like I said, you have not provided the definition of Value<T>, so I can only make a guess. Well, it turned out that I was wrong. You can ignore that paragraph, since what it is based on is incorrect. \$\endgroup\$ – Xiaoy312 Sep 19 '16 at 0:19
2
\$\begingroup\$

This is a very bad design:

public virtual void Update(object sender, EventArgs e)
{

}

The default implementaiton not only doesn't do anthing but it also has a bad signature.

If you are not doing anything you should make it abstract or at least put a comment there explaining why you are not implementing it.

You should make this method protected because you clearly use it in one of the constructors. I don't think anyone else should be able to call it.

LostFocus += new EventHandler(Update);

This is where you actually use the Update method as an event handler. But its name doesn't match the event. The correct name should be TextBox_LostFocus.

So going this path you should now have this:

private void TextBox_LostFocus(object sender, EventArgs e)
{
     Update();
}

and

LostFocus += TextBox_LostFocus;

Now you see that the actual method that you want to let the derived classes implement is the pure Update method that should now have this signature:

protected abstract void Update();

The derived text boxes should now only care about this method and implement it according to their needs.

Because the new Update method isn't an event handler anymore, you could even make it public now if you need to.


Imagine you used the sender parameter of the old Update and you did inside it the following:

var textBox = sender as TextBox;

and you start updating the text box but the update was called not by the texbox implementing it but some other text box because someone misused it. The update wouldn't update this one but the other text box! Happy bugs hunting ;-)

\$\endgroup\$
  • \$\begingroup\$ Since I condensed it all into once class, I didn't have to deal with inheritance anymore. But I did reorganize the Update() according to your advice. Thanks! \$\endgroup\$ – Karlovsky120 Sep 16 '16 at 16:18

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.