HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Dynamic display of number converted to different numeral system

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
numbersystemdifferentnumeraldynamicdisplayconverted

Problem

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.

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

Solution

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.

Context

StackExchange Code Review Q#79231, answer score: 5

Revisions (0)

No revisions yet.