21
\$\begingroup\$

I recently started "coding" and this is one of my first "projects". It is supposed to be an SI converter where you can type a value, its unit and the unit to which you want it to be converted.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Program
{
    class Program
    {
        static void Main()
        {


            decimal one = 1;
            decimal two = 0.001m;
            decimal three = 0.000001m;
            decimal four = 0.000000001m;
            decimal five = 0.000000000001m;
            decimal answer;
            int y = 0;

            while (y < 1)
            {


                Console.WriteLine("SI converter!\nPlease, enter value: ");
                decimal value = Convert.ToInt32(Console.ReadLine());

                Console.WriteLine("\nFactors: \n1.One \n2.Milli(m)\n3.Micro(µ)\n4.Nano(n)\n5.Pico(p)\nEnter factor: ");
                decimal factor = int.Parse(Console.ReadLine());

                if (factor == 1)
                {
                    factor = one;
                }
                else if (factor == 2)
                {
                    factor = two;
                }
                else if (factor == 3)
                {
                    factor = three;
                }
                else if (factor == 4)
                {
                    factor = four;
                }
                else if (factor == 5)
                {
                    factor = five;
                }


                Console.WriteLine("\nFactors: \n1.One \n2.Milli(m)\n3.Micro(µ)\n4.Nano(n)\n5.Pico(p)\nEnter the second factor: ");
                decimal factor2 = Convert.ToInt32(Console.ReadLine());

                if (factor2 == 1)
                {
                    factor2 = one;
                    answer = value * factor;
                    Console.WriteLine("The answer is : " + answer);
                }
                else if (factor2 == 2)
                {
                    factor2 = two;
                }
                else if (factor2 == 3)
                {
                    factor2 = three;
                }
                else if (factor2 == 4)
                {
                    factor2 = four;
                }
                else if (factor2 == 5)
                {
                    factor2 = five;
                }



                answer = value * factor / factor2;
                Console.WriteLine("The answer is : " + answer);


                Console.WriteLine("Go again?\nY / N");
                char ans = char.Parse(Console.ReadLine());
                if (ans == 'n')
                {
                    y = 8;
                }
                if (ans == 'y')
                {

                    y = 0;
                    Console.Clear();

                }
            }
        }
    }
}
    }

Can you give me some opinions about the code and how I can improve?

\$\endgroup\$
16
\$\begingroup\$

I'd just start making your factor namings consistent with the SI factors, like

        decimal unit = 1;
        decimal milli = 0.001m;
        decimal micro = 0.000001m;
        decimal nano = 0.000000001m;
        decimal pico = 0.000000000001m;

And there are even more like

        decimal kilo = 1000.0;
        decimal mega = 1000000.0;
        decimal deci = 0.1m;
        decimal centi = 0.01m;

This would make your code's intent much more readable and concise when having statements as follows:

if (factor == 1) {
    factor = unit;
}

Also I'd separate users choice and factors chosen go to different variables like:

            int choice = Convert.ToInt32(Console.ReadLine());

            switch(choice) {
            case 1:
                factor = unit;
                break;
            case 2:
                factor = milli;
                break;
            // etc.
            }

To make a clear separation between user input and the actual factors chosen. That will make the code even more readable, maintainable and concise.


Also, since you've been asking for c# specifically, you can have the user input as a string directly, without need to map from a number:

Console.WriteLine("\nFactors: \nUnit \nMilli(m)\nMicro(µ)\nNano(n)\nPico(p)\nEnter factor: ");
string choice = Console.ReadLine();

switch(choice) {
case "Unit":
    factor = unit;
    break;
case "Milli":
    factor = milli;
    break;
// etc.
}

or alternatively lookup the factor values from a properly initialized Dictonary<string, double>.

\$\endgroup\$
  • \$\begingroup\$ Yes I'm thinking about doing the rest (Kilo, Mega, Giga ,Tera ) but I don't know if I can do it :D \$\endgroup\$ – BatmanB. Dec 2 '15 at 19:33
  • \$\begingroup\$ I tried but I can't combine decimal with switch... Or at least I don't know how. \$\endgroup\$ – BatmanB. Dec 2 '15 at 19:34
  • \$\begingroup\$ The switch is just what you're asking the user and what unit the want to have converted into another. So add just more numbers, and interpretet these mapping to factors internally. In general keep that input not directly related to values given from input. \$\endgroup\$ – πάντα ῥεῖ Dec 2 '15 at 19:40
  • \$\begingroup\$ @BatmanB. I was just answering from a straight forward POV how to improve your existing code. Of course for something production ready, you should go with Caridorc's generic solution, which avoids if/else or swich/case at all. \$\endgroup\$ – πάντα ῥεῖ Dec 2 '15 at 20:07
  • \$\begingroup\$ if (factor == 1) { factor = unit; } seems kinda useless. I don't know if it's that good to promote it. Aside from that, great first answer! Welcome! \$\endgroup\$ – Fund Monica's Lawsuit Dec 3 '15 at 0:36
13
\$\begingroup\$

I suggest a simplification from:

        decimal one = 1;
        decimal two = 0.001m;
        decimal three = 0.000001m;
        decimal four = 0.000000001m;
        decimal five = 0.000000000001m;
        decimal answer;
        int y = 0;

        while (y < 1)
        {


            Console.WriteLine("SI converter!\nPlease, enter value: ");
            decimal value = Convert.ToInt32(Console.ReadLine());

            Console.WriteLine("\nFactors: \n1.One \n2.Milli(m)\n3.Micro(µ)\n4.Nano(n)\n5.Pico(p)\nEnter factor: ");
            decimal factor = int.Parse(Console.ReadLine());

            if (factor == 1)
            {
                factor = one;
            }
            else if (factor == 2)
            {
                factor = two;
            }
            else if (factor == 3)
            {
                factor = three;
            }
            else if (factor == 4)
            {
                factor = four;
            }
            else if (factor == 5)
            {
                factor = five;
            }

to

int y = 0

while (y < 1)
{
    Console.WriteLine("SI converter!\nPlease, enter value: ");
    decimal value = Convert.ToInt32(Console.ReadLine());

    Console.WriteLine("\nFactors: \n1.One \n2.Milli(m)\n3.Micro(µ)\n4.Nano(n)\n5.Pico(p)\nEnter factor: ");
    decimal exponent = int.Parse(Console.ReadLine());

    factor = Math.pow( 10, ( - ( (exponent - 1) * 3 ) ) )

This makes use of the fact that the coefficient are evenly spaced on a log scale, where each coefficient is the previous to the -10**3

Some context

A number \$x\$ in scientific notation is in the form:

$$x = a * Pow(10, b)$$

  • Where \$a\$ is a number from \$0\$ to \$9.9\$
  • Where \$b\$ is an integer

Now the various prefixes that you listed in your program are each one \$1000\$ times smaller than the other.

This means that to get from one prefix to the next you have to multiply it by \$Pow(10, -3)\$

So given n you multiply it by \$Pow(10, -3 * n)\$ where \$n\$ is the input number and you get the \$n-th\$ suffix.

The \$- 1\$ just goes from 1-indexing to 0-indexing.


And a simplification in the form of removing the y variable completely and modifying the end to:

        if (ans == 'n')
        {
            return;
        }
        if (ans == 'y')
        {
            Console.Clear();
        }

Returning immediately is also easier to understand that setting a counter declared tens of lines ago to 8 (just out of curiosity: why 8 and not 1 or 2?)

\$\endgroup\$
  • \$\begingroup\$ Wow that is way more simple than mine! Though I can't really understand it :D \$\endgroup\$ – BatmanB. Dec 2 '15 at 19:24
  • \$\begingroup\$ @BatmanB. it is easier than it looks, do you know scientific notation? \$\endgroup\$ – Caridorc Dec 2 '15 at 19:25
  • \$\begingroup\$ Kinda ... we are doing "transformations"? with SI units in school so I decided I can try to make some kind of converter which can do it for me :D \$\endgroup\$ – BatmanB. Dec 2 '15 at 19:29
  • \$\begingroup\$ @BatmanB. I added some context tell me if it is any clearer \$\endgroup\$ – Caridorc Dec 2 '15 at 19:36
  • 1
    \$\begingroup\$ @Caridorc Thanks for the explanation I pretty much got it...(about the 8 I don't really know why I just did it \$\endgroup\$ – BatmanB. Dec 2 '15 at 19:45
10
\$\begingroup\$

This is going to be a longer example, and I'm just dealing with the conversion logic here because I think it's the most instructive. It's also not a "real world" example in the sense that the specific case you provided doesn't justify anything close to this much structure, but you could very easily find yourself heading in this direction in a production-scale app.

The code I've provided can be used to handle the input and the desired factor.

See the conversion and multiple codes here

Key Points

  • Again, this is a "teaching example". As a beginner you should not expect your code to look anything like this. One step at a time :)
  • Related data belongs together in the same type. The multiple doesn't make sense without it's prefix name.

Classes!

It's generally a good idea to create types that represents sets of values that belong together. The SI multiples are example of this. The name "nano" is associated with its factor. Because they all form a composite entity or idea, they make the most sense in a type (struct or class), which is the Multiple type in the code I provided.

public struct Multiple
{
    public Multiple(string name, int exponent)
    {
        Name = name;
        Exponent = exponent;
    }
    public string Name { get; }  // Read-Only Property
    public int Exponent { get; } // Read-Only Property
    public double Multipler => Math.Pow(10, Exponent);
    public double ConvertToBaseValue(double inputValue)
    {
        return inputValue/Multipler;
    }
    public double ConvertFromBaseValue(double inputValue)
    {
        return inputValue*Multipler;
    }
    public UnitValue CreateValue(double inputValue)
    {
        var newBaseValue = ConvertToBaseValue(inputValue);
        return new UnitValue(this, newBaseValue);
    }
    public static Multiple Singular => new Multiple(null, 0);
    public static Multiple Milli => new Multiple("Milli", -3);
    public static Multiple Micro => new Multiple("Micro", -6);
    public static Multiple Nano => new Multiple("Nano", -9);
    public static Multiple Pico => new Multiple("Pico", -12);
}

Similarly, your input with two questions (what is the value? what unit do I want to see it in?) then forms the basis of the UnitValue type. If you separate the two from each other they don't make a lot of sense.

public class UnitValue
{
    public UnitValue(Multiple multiple, double baseValue)
    {
        Multiple = multiple;
        BaseValue = baseValue;
    }
    public Multiple Multiple { get; }
    public double PrefixedValue
    {
        get { return Multiple.ConvertFromBaseValue(BaseValue); }
        set { BaseValue = Multiple.ConvertToBaseValue(value); }
    }
    public double BaseValue { get; set; }
    public UnitValue ConvertTo(Multiple multiple)
    {
        return new UnitValue(multiple, BaseValue);
    }
}

Static/Class Methods for Default Values

I've added factory static methods to the Multiple to standardize the values. You'd want all of the standard definitions built into the type, or closely nearby.

    // Members of Multiple type.
    public static Multiple Singular => new Multiple(null, 0);
    public static Multiple Milli => new Multiple("Milli", -3);
    public static Multiple Micro => new Multiple("Micro", -6);
    public static Multiple Nano => new Multiple("Nano", -9);
    public static Multiple Pico => new Multiple("Pico", -12);
    // Allows you to get multiples like so.
    var m = Multiple.Milli;
    var u = Multiple.Micro;

Don't Repeat Yourself!

Everyone will say this over and over again. Here I think it's demonstrated by avoiding typing in tedious values with all those zeroes. It also avoids bugs where I had 0.0001 for the milli prefix.

    // Risky
    public static Multiple Pico => new Multiple("Pico", 0.000000000001);
    // Safer
    public static Multiple Pico => new Multiple("Pico", -12);

Property Accessors

Instead of making the user use a special method to update the base value or prefixed value, I've made the decision to make the BaseValue property an auto-property ( which means I don't need to write boilerplate code for the gettor and settors), and had the PrefixedValue wired up to it. This way you can make the lives of the users of the type somewhat easier, at the price of making them responsible to know when to use the different values and why.

    public double PrefixedValue
    {
        get { return Multiple.ConvertFromBaseValue(BaseValue); }
        set { BaseValue = Multiple.ConvertToBaseValue(value); }
    }

Get Only Properties

Because the Multiple data types belong together, it doesn't make any sense for the client to update them. Therefore to prevent data corruption I've made them get only properties that are set at the time the object is instantiated.

    // Constructor assigns the values when instantiated.
    public Multiple(string name, int exponent)
    {
        Name = name;
        Exponent = exponent;
    }
    public string Name { get; }  // Can only be set when instantiated.
    public int Exponent { get; } // Can only be set when instantiated.

Summary

These are some of the things you'd want to consider as you move forward. I've skipped other things that are important to understand, but they can wait until you have a lot more experience:

  • Program to the interface, not to the implementation.
  • Using inheritance to specialize (if I wanted to handle imperial weights).
  • Using reflection to iterate over all the predefined multiples so you could change the defined multiples and your UI would update automatically.
  • Unit Testing your code to validate design.

I hope that wasn't too scary! If anyone has any feedback I'd love to hear it.

Edit 1 : Adding Usage Example

I've amended my gist here to include an example of the Main program, if it were using the classes I've written. I didn't go through a full refactoring exercise there except to remove duplication, and to push the specifics of logging into the Log method.

Why Replace Console.WriteLine()? It's a Platform Function

Indeed it is. But you're still repeating it all over the place. Imagine if you wanted to change the way you log things? Or add a standard format?

If you keep the implementation away from the users of the logging function, then you can change it whenever you want. If you wanted to replace the console logging function with a log file, you could change it in one place. Without this abstraction you'd be updating literally dozens of references.

    // "Brittle" Code
    Console.WriteLine("Bob");  // In one file
    Console.WriteLine("Says"); // In another file
    Console.WriteLine("Hi!");  // In a third file

    // Abstracted Code
    Log("Bob");
    Log("Bob");

    private static void Log(string message)
    {
        Console.WriteLine(message);
    }

One Final Thing You also want to consider what happens if the user types something weird in. This example doesn't include any error handling around user entry.

\$\endgroup\$
  • 1
    \$\begingroup\$ A bit too unspecific without leaving examples for a beginner, that all might just go beyond their already gained concepts so far. Though I agree with all of the points you've given. \$\endgroup\$ – πάντα ῥεῖ Dec 2 '15 at 20:36
  • \$\begingroup\$ Thanks @πάνταῥεῖ ! I've amended the gist accordingly, and have added the Main.cs file with my changes. \$\endgroup\$ – Rich Seviora Dec 3 '15 at 0:21
  • 1
    \$\begingroup\$ @RubberDuck You're definitely right :) That's now been done. \$\endgroup\$ – Rich Seviora Dec 3 '15 at 1:59
  • \$\begingroup\$ In my opinion this is best answer here. To fix just few minor issues when there is a bigger design flaw is useless. Especially because OP is beginner then he should know how things should be done otherwise...what's the point to ask on CR?! \$\endgroup\$ – Adriano Repetti Dec 3 '15 at 8:57
  • \$\begingroup\$ Thanks but I can't really get the classes... \$\endgroup\$ – BatmanB. Dec 3 '15 at 12:08

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.