6
\$\begingroup\$

The program is displaying given number (right now it is only signed int) converted to different numeral system (binary,octal and hexadecimal for now). Number can be input in the text box and the display updates automatically in the ConvrterDisplay UserControl. Form has a possibility to enable autoincrement provied by timer. There was more description but I think that simple screenshot can do here well.

Screenshot of program

So thats pretty much it. Trying on improving my skills so thats like tiny training program and I'd like to know if theres some principal I might break here or just coded something ankwardly.

So lets start from the inside. First, there's abstract converter to return desired value and actual converters inheriting from it.

public abstract class AbstractNumberConverter {

    public abstract string GetString (int inputValue);
}

public class BinaryConverter:AbstractNumberConverter {

    public override string GetString (int inputNumber) {
        return Convert.ToString(inputNumber, 2);
    }
}

public class OctalConverter:AbstractNumberConverter {

    public override string GetString (int inputValue) {
        return Convert.ToString (inputValue, 8);
    }
}

public class HexadecimalConverter:AbstractNumberConverter {

    public override string GetString (int inputValue) {
        return Convert.ToString(inputValue, 16).ToUpper();
    }
}

Then comes the factory to provide correct conversion and enum to make it more readable.

public class ConverterFactory {

    public AbstractNumberConverter GetConverter (NumericSystems format) {

        switch (format) {
            case NumericSystems.Octal:
                return new OctalConverter ();

            case NumericSystems.Hexadecimal:
                return new HexadecimalConverter ();

            default:
                return new BinaryConverter ();  
        }
    }
}

public enum NumericSystems {
    Binary = 2,
    Octal = 8,
    Hexadecimal = 16
}

Now the time for the UI, I don't want to mess so I'll skip most of XAML and just let you know about the bindings.

DisplayField is responsible for displaying given char with it's base

public partial class DisplayField : UserControl, INotifyPropertyChanged {

    private int _numberBase;
    private int _numberPower;
    private char _numberValue;


    public int NumberBase {
        get { return _numberBase; }
        set {
            if (value == _numberBase) return;
            _numberBase = value;
            OnPropertyChanged ();
        }
    }

    public int NumberPower {
        get { return _numberPower; }
        set {
            if (value == _numberPower) return;
            _numberPower = value;
            OnPropertyChanged ();
        }
    }

    public char NumberValue {
        get { return _numberValue; }
        set {
            if (value == _numberValue) return;
            _numberValue = value;
            OnPropertyChanged ();
        }
    }
    public DisplayField (int numberBase, int numberPower) {
        _numberBase = numberBase;
        _numberPower = numberPower;

        InitializeComponent ();
    }

    public event PropertyChangedEventHandler PropertyChanged;

    [NotifyPropertyChangedInvocator]
    protected virtual void OnPropertyChanged ([CallerMemberName] string propertyName = null) {
        PropertyChanged?.Invoke (this, new PropertyChangedEventArgs (propertyName));
    }
}

ConverterDisplay is the contol I use to display output in the tab contol, the most important were dependency properties here so all the changes go through binding.

<converters:ConverterDisplay    x:Name="binConverter"
                                OutputFormat="Binary"
                                InputValue="{Binding ElementName=Window, Path=CurrentValue, Mode=OneWay}"/>

I'm curious if there's other way that I can call UpdateDisplay and UpdateConverter when dependency properties changes? Wanted to have UpdateConverter in constructor but then I discovered that Properties you set through XAML are applied little bit late so I was ending up with default converters regardless of the OutputFormat I set in XAML.

 public partial class ConverterDisplay : UserControl {

    private const int DEFAULT_INPUT_VALUE = 0;
    private AbstractNumberConverter _converter;
    private readonly DisplayFieldListManager _fieldManager;

    public static readonly DependencyProperty InputValueProperty = DependencyProperty.Register (
        "InputValue",
        typeof(int),
        typeof(ConverterDisplay),
        new FrameworkPropertyMetadata (DEFAULT_INPUT_VALUE, InputValuePropertyChanged));

    public static readonly DependencyProperty OutputFormatProperty = DependencyProperty.Register (
        "OutputFormat",
        typeof(NumericSystems),
        typeof(ConverterDisplay),
        new FrameworkPropertyMetadata ( OutputFormatPropertyChanged));


    public int InputValue {
        get { return (int) GetValue (InputValueProperty); }
        set { SetValue (InputValueProperty, value); }
    }

    public NumericSystems OutputFormat {
        get { return (NumericSystems) GetValue (OutputFormatProperty); }
        set { SetValue (OutputFormatProperty, value); }
    }

    public ObservableCollection <DisplayField> DisplayFields { get; }

    public ConverterDisplay () {
        DisplayFields = new ObservableCollection <DisplayField> ();
        _fieldManager = new DisplayFieldListManager (DisplayFields);
        _converter    = new BinaryConverter ();

        InitializeComponent ();
        UpdateConverter ();
    }

    private void UpdateConverter () {
        _converter = new ConverterFactory ().GetConverter (OutputFormat);
        _fieldManager.ResetDisplay ();
        UpdateDisplay ();
    }

    private void UpdateDisplay () {
        var convertedNumberString = _converter.GetString (InputValue);

        _fieldManager.UpdateDisplayFields (convertedNumberString, OutputFormat);
    }

    private static void InputValuePropertyChanged (DependencyObject dependencyObject,
        DependencyPropertyChangedEventArgs dependencyPropertyChangedEventArgs) {
        var instance = (ConverterDisplay) dependencyObject;
        if (instance != null)
            instance.UpdateDisplay ();
    }

    private static void OutputFormatPropertyChanged (DependencyObject dependencyObject,
        DependencyPropertyChangedEventArgs dependencyPropertyChangedEventArgs) {
        var instance = (ConverterDisplay) dependencyObject;
        if (instance != null)
            instance.UpdateConverter ();
    }
}

DisplayFieldListManager takes responsibility for any actions related to the collection of DisplayField, extracted it from ConverterDisplay at some point. UpdateDisplayFields is called every time I need to refresh output, list is supposed to have number of DisplayField just to dispay current convertedNumberString.

public class DisplayFieldListManager {

    private readonly ObservableCollection <DisplayField> _fields;

    public DisplayFieldListManager (ObservableCollection <DisplayField> fields) {
        _fields = fields;
    }

    public void ResetDisplay () {
        _fields.Clear ();
    }

    public void UpdateDisplayFields (string convertedNumberString, NumericSystems format) {
        if (TooManyFields (convertedNumberString))
            ResetDisplay ();

        while (NotEnoughFields (convertedNumberString))
            AddDisplayField (format);

        UpdateValues (convertedNumberString);
    }

    private bool NotEnoughFields (string convertedNumberString) {
        return _fields.Count < convertedNumberString.Length;
    }

    private bool TooManyFields (string convertedNumberString) {
        return _fields.Count > convertedNumberString.Length;
    }

    private void UpdateValues (string convertedString) {
        if (_fields.Count == 0)
            return;

        for (var i = 0; i < _fields.Count; i++) {
            _fields [i].NumberValue = convertedString [i];
        }
    }

    private void AddDisplayField (NumericSystems format) {
        _fields.Insert (
            0,
            new DisplayField ((int) format, _fields.Count));
    }
}

Last part is just responsible for updating CurrentValue in manual way where user just writes it in the TextBox bound to the CurrentValue or in automatic mode it gets increased by ticking timer (interval can be adjusted with Slider, thats why I exposed TimerSpeed). AutomaticMode is bound with RadioButtons.

public partial class MainWindow : Window, INotifyPropertyChanged {

    private int _currentValue;
    private bool _automaticMode;
    private DispatcherTimer _timer;

    public int CurrentValue {
        get { return _currentValue; }
        set {
            if (value == _currentValue) return;
            _currentValue = value;
            OnPropertyChanged ();
        }
    }

    public double TimerSpeed {
        get {
            if (_timer == null) return 0;
            return _timer.Interval.TotalSeconds;
        }
        set {
            if (_timer == null) return;
            _timer.Interval = TimeSpan.FromSeconds (value);
            OnPropertyChanged ();
        }
    }

    public bool AutomaticMode {
        get { return _automaticMode; }
        set {
            if (value.Equals (_automaticMode)) return;
            _automaticMode = value;
            ManipulateTimer ();
            OnPropertyChanged ();
        }
    }

    public MainWindow () {
        InitializeComponent ();
        InitializeTimer ();
    }

    protected virtual void ResetValue () {
        CurrentValue = 0;
    }

    private void InitializeTimer () {
        _timer = new DispatcherTimer ();
        TimerSpeed = .1;
        _timer.Tick += _timer_Tick;
    }

    private void ManipulateTimer () {
        if (_automaticMode)
            _timer.Start ();
        else
            _timer.Stop ();
    }

    private void _timer_Tick (object sender, EventArgs e) {
        CurrentValue++;
    }

    private void btnReset_Clicked (object sender, RoutedEventArgs e) {
        ResetValue ();
    }

    private void btnMaxValue_Clicked (object sender, RoutedEventArgs e) {
        CurrentValue = int.MaxValue;
    }
    public event PropertyChangedEventHandler PropertyChanged;

    [NotifyPropertyChangedInvocator]
    protected virtual void OnPropertyChanged ([CallerMemberName] string propertyName = null) {
        PropertyChanged?.Invoke (this, new PropertyChangedEventArgs (propertyName));
    }
}
\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

An abstract class which only contains one abstract method is screaming to be an interface. There is absolutely no advantage for AbstractNumberConverter to be an abstract class.


The items of the enum NumericSystems don't need any values assigned. You won't gain any advantage from assigning values, because you don't use the values.


You should use braces {} for single if statements, single statements of while and for loops. This will help you by making your code less errorprone.
If you decide to not use them, you should stick to your choosen style. Right now you are mixing your style. Sometimes you use braces sometimes you don't.


Speaking about style, most C# developers are placing the opening brace { on a new line. Your style just looks more like java.


I like that for classes which are implementing INotifyPropertyChanged you only raise this event for the case that a set property value is unequal the current one.


I like, that you leave some space to breathe for your variables. But you should do this also for the classname and the extended/implemented abstract class.

\$\endgroup\$
2
  • \$\begingroup\$ Hey Heslacher! Thanks for your time here. I'm really glad you mentioned being constant about the style, I have noticed that sometimes I just rush with some method and forget about things like that or decide to do it later, ending up forgetting about that. So I'll try to focus more on that. I actually use 'NumericSystems' values in the 'AddDisplayField' method. Is that alright? Could you explain what do you mean by leaving some space to breathe for class names? \$\endgroup\$
    – yoger
    Commented Feb 2, 2015 at 15:56
  • 1
    \$\begingroup\$ I mean BinaryConverter:AbstractNumberConverter should be BinaryConverter : AbstractNumberConverter \$\endgroup\$
    – Heslacher
    Commented Feb 2, 2015 at 15:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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