Cfrtjryk

I am creating a very small application to demonstrate solid principles and also a brief implementation of a builder pattern, does anyone have any feedback as to how this could be improved or how it breaks the solid principles? The app is a small app that just converts units, e.g. yards to meters, inches to cms.

Interface

public interface IConverter

{

    double ConversionRate { get; set; }

    string ConvertedUnits { get; set; }

    string DisplayText { get; set; }



    string Convert(string input);

}

Class implementing interface

 public class Converter : IConverter

    {

        public double ConversionRate { get; set; }

        public string ConvertedUnits { get; set; }

        public string DisplayText { get; set; }



        public Converter(double conversionRate, string convertedUnits, string displayText)

        {

            ConversionRate = conversionRate;

            ConvertedUnits = convertedUnits;

            DisplayText = displayText;

        }



        public string Convert(string input)

        {

            if (!input.Contains('n'))

            {

                double d = double.Parse(input, CultureInfo.InvariantCulture) * ConversionRate;



                string array = new string[1];

                array[0] = d.ToString();



                return array;

            }

            else

            {

                double doubles = new double[input.Split('n').Count()];



                for (int i = 0; i < input.Split('n').Count(); i++)

                {

                    double value = double.Parse(input.Split('n')[i]);

                    doubles[i] = value;



                    string.Format("{0}", value * ConversionRate);

                }



                string strings = new string[doubles.Length];



                for (int i = 0; i < input.Split('n').Length; i++)

                {

                    strings[i] = string.Format("{0}", doubles[i] * ConversionRate);

                }



                return strings;

            }

        }

    }

Builder (abstract class)

public abstract class ConverterBuilder

    {

        protected double _conversionRate;

        protected string _convertedUnits;

        protected string _displayText;



        public ConverterBuilder AddConversionRate(double conversionRate)

        {

            _conversionRate = conversionRate;

            return this;

        }



        public ConverterBuilder AddConversionRate(string convertedUnits)

        {

            _convertedUnits = convertedUnits;

            return this;

        }



        public ConverterBuilder AddDisplayText(string displayText)

        {

            _displayText = displayText;

            return this;

        }



        public Converter Build()

        {

            return new Converter(_conversionRate, _convertedUnits, _displayText);

        }



    }

Builder implementing abstract builder class

public class UnitConverterBuilder : ConverterBuilder

{

    public UnitConverterBuilder()

    {



    }

}

Controller

public IActionResult Convert(string text, double conversionRate)

{

    Converter converter = new UnitConverterBuilder()

    .AddConversionRate(conversionRate)

    .Build();



    string output = "";

    for (int i = 0; i < converter.Convert(text).Count(); i++)

    {

        output += converter.Convert(text)[i] + Environment.NewLine;

    }



    return Content(output);



}

View

Enter values to convert

<form asp-controller="Home" asp-action="Convert" method="post">

    <textarea id="text" name="text" rows="10" cols="40"></textarea>

    <select name="conversionRate">

        <option value="">Please Select</option>

        <option value="0.9144">Yards to Meters</option>

        <option value="2.54">Inches To Centimeters</option>

    </select>

    <button>Convert</button>

</form>

The app is built using .net core, any feedback greatly appreciated :)

The S in SOLID

Since you mention SOLID, I'd like to point out that IConverter has too many responsibilities:

1. It converts from one unit to another.


2. It splits input on newlines


3. It contains display-related information.

The first is exactly what you'd expect a converter to do.

The second is better left to the caller. If they need to convert multiple inputs they can simply call IConverter.Convert multiple times. You already need to do that anyway if you've got a comma-separated string or an array of inputs, so why should the conversion logic contain a special case for newlines?

The third should be the responsibility of the UI layer of your program, where you can take care of things like localization and visualization.

Scalable design

Another issue with this design is that it's not very scalable. Every 'unit-to-unit' conversion requires a different converter instance. With just inches, centimeters and millimeters, you already need 6 converters: inch->cm, inch->mm, cm->inch, cm->mm, mm->inch and mm->cm. For 10 units, you'll need 90.

A better design would define each unit in terms of a single base unit. For example, taking centimeter as base: Unit("inch", 2.54), Unit("cm", 1.0), Unit("mm", 0.1). That gives you enough information to be able to convert from any of these units to any other. Adding another unit now only requires a single new piece of information.

Other issues

• Those IConverter properties should not have public setters - you don't want other code to be able to modify the conversion rate of a converter!


• There's no documentation, and several things aren't clear from the code itself: the purpose of that ConvertedUnits array, or why the return type of Convert is an array of strings, among other things.


• The builder contains two AddConversionRate methods - one of them should be renamed to AddConvertedUnits.


• This has been pointed out already: you're repeating work by calling input.Split('n') too many times. But the same goes for the controller code: you're also repeatedly calling converter.Convert.


• A related issue is that of code duplication: in Convert, the if part is doing the same as the else part, minus the string-splitting. However, if the input does not contain any newlines then string.Split will return an array with one element, so the else part alone is sufficient.

Simplicity

Finally, I agree with what Konrad Rudolph said in the comments: what's the benefit of using an interface and a builder here? This sort of design is very useful when, for example, you're working with a database: it allows you to swap DatabaseConnection for SimulatedDatabaseConnection while testing. But can you think of a situation where being able to swap Converter for another implementation would be useful?

Keep in mind that this extra flexibility doesn't come for free - there's more code to maintain, and more complex designs are also more difficult to maintain. So first ask yourself: do I need this kind of flexibility, and is it worth the cost?

In this case, a simpler design will likely suffice:

public class Unit

{

    public string Name { get; }

    public double Ratio { get; }



    public Unit(string name, double ratio)

    {

        Name = name;

        Ratio = ratio;

    }



    /// <summary>

    /// Converts a value from the current unit to the specified unit.

    /// </summary>

    public double Convert(double value, Unit toUnit)

    {

        return (value * Ratio) / toUnit.Ratio;

    }

}

It makes sense to combine that with a Unit GetUnit(string name) 'factory method', so you don't need to modify calling code when adding a unit. GetUnit could load units from a database, a file, or simply contain a few hard-coded ones - details that calling code shouldn't need to worry about.

A Unit AvailableUnits() method (or property) would also be useful, if you want to show users which units they can convert between. That's better than having the user type out the unit name, and then wonder why "centimeter" doesn't work but "cm" does.

// Basic conversion, inch to cm:

var convertedValue = GetUnit("inch").Convert(value, toUnit: GetUnit("cm"));



// Your controller code, refactored - including some error handling:

var fromUnit = AvailableUnits[fromUnitIndex];

var toUnit = AvailableUnits[toUnitIndex];

return string.Join(Environment.NewLine,

    input.Split('n')

        .Select(part =>

        {

            if (double.TryParse(part, out var value))

                return fromUnit.Convert(value, toUnit).ToString();

            else

                return $"Invalid input: '{part}'";

        }));

Only targeting public string Convert(string input)

• Because this is a public method you should validate the input. Right now if one passes null this will throw an ArgumentNullException which isn't that bad but the stacktrace belongs to System.Linq.Enumerable.Contains() which you should hide.
  
  Just testing against null or using string.IsNullOrWhiteSpace()` will do the trick.


• You are calling input.Split('n') many many times. It would be much better to call it once and store the result in a variable.


• Don't use Count() method on an array, Using Count() use the Length property instead.


• You should use the overloaded Split() method which takes a [StringSplitOptions][2] enum like StringSplitOptions.RemoveEmptyEntries.


• Instead of using double.Parse() you should consider to use double.TryParse() which won't throw an exception if the current string doesn't represent a double like e.g a letter.


• This string.Format("{0}", value * ConversionRate); can be safely removed because the result isn't assigned to anything.


• Instead of returning a string you should consider to use an IEnumerable<string or better just an IEnumerable<double> which is more straightforward. Sure that means you need to change your interface as well.


• Using a foreach will make the code shorter because you won't need to check for n.

Implementing the mentioned points will lead to

string version

public IEnumerable<string> Convert(string input)

{

    if (string.IsNullOrWhiteSpace(input))

    {

        yield break;

    }

    var values = input.Split(new char { 'n' }, StringSplitOptions.RemoveEmptyEntries);

    foreach (var value in values)

    {

        double current;

        if (double.TryParse(value, NumberStyles.Any, CultureInfo.InvariantCulture, out current))

        {

            yield return (current * ConversionRate).ToString();

        }

    }

}

double version

    public IEnumerable<double> Convert(string input)

    {

        if (string.IsNullOrWhiteSpace(input))

        {

            yield break;

        }

        var values = input.Split(new char { 'n' }, StringSplitOptions.RemoveEmptyEntries);

        foreach (var value in values)

        {

            double current;

            if (double.TryParse(value, NumberStyles.Any, CultureInfo.InvariantCulture, out current))

            {

                yield return (current * ConversionRate);

            }

        }

    }

Ok, I need to target Controller.Convert() as well

This is really really bad:

string output = "";

for (int i = 0; i < converter.Convert(text).Count(); i++)

{

    output += converter.Convert(text)[i] + Environment.NewLine;

}



return Content(output);  

Assume you have text = "n1n2n3n4n5.......n1000" then your code will once call converter.Convert() at
for (int i = 0; i < converter.Convert(text).Count(); i++)

which results in Count() = 1000 hence the loop will be executed 1000 times which calls converter.Convert(text) 1000 times.
In addition using output += on strings will lead to very bad performance because each time a new string object is created because strings are immutable.

Better use a StringBuilder.

Assuming you use the IEnumerable<double> Convert() this will lead to

    StringBuilder sb = new StringBuilder(1024);

    foreach (var value in converter.Convert(text))

    {

        sb.AppendLine(value.ToString());

    }



    return Content(sb.ToString());