Krdytkyu

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 :)

public IActionResult Convert(string text, double conversionRate)

{

    Converter converter = new UnitConverterBuilder()

    .AddConversionRate(conversionRate)

    .Build();

Here you should use IConverter converter = ... instead of Converter converter = .... That is the whole idea of having an interface. And maybe ConverterBuilder.Build() should return an IConverter interface instead of the Converter class?

I think your Convert(...) method lacks to deal with strings that are not parsable to double. It will throw a FormatException on the first invalid number string. So you must at least provide a try-catch block around the calls to Convert() in the calling method. Heslacher suggest using double.TryGetValue(), but does it IMO only halfway, because he just ignores strings that are not convertible. In that way the client has no idea of what went wrong where. Using Haslachers approach you need to do something like:

    double current;

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

    {

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

    }

    else

    {

       yield return double.NaN.ToString();

    }

or otherwise signal to the client that something went wrong, because if you enter a string with say 5 values and one is in invalid format you'll only get 4 back, but which 4?

I can't quite figure out why you declare ConverterBuilder as abstract? Why can't consumers use it as is? It is fully working and requires no extensions in derived objects to work properly. To give it any meaning you should make the methods virtual so subclasses can extent/alter the default behavior.

Further:

An abstract class with only non-virtual/non-abstract methods is only meaningful by letting subclasses extent the behavior:

  public class UnitConverterBuilder : ConverterBuilder

  {

    public UnitConverterBuilder()

    {



    }



    public UnitConverterBuilder AddSomethingElse(object somethingElse)

    {

      return this;

    }



  }

your method chaining can then become rather complicated and hard to read and understand:

  UnitConverterBuilder builder = new UnitConverterBuilder();

  IConverter converter = (builder

    .AddConversionRate(2.54)

    .AddDisplayText("Inches to cm") as UnitConverterBuilder)

    .AddSomethingElse("something else")

    .Build();

because the methods return the base class ConverterBuilder.

You can overcome this by defining the base class as:

  public abstract class ConverterBuilder<TSubClass> where TSubClass : ConverterBuilder<TSubClass>

  {

    protected double _conversionRate;

    protected string _convertedUnits;

    protected string _displayText;



    public virtual TSubClass AddConversionRate(double conversionRate)

    {

      _conversionRate = conversionRate;

      return this as TSubClass;

    }



    public virtual TSubClass AddConversionRate(string convertedUnits)

    {

      _convertedUnits = convertedUnits;

      return this as TSubClass;

    }



    public virtual TSubClass AddDisplayText(string displayText)

    {

      _displayText = displayText;

      return this as TSubClass;

    }



    public virtual IConverter Build()

    {

      return new Converter(_conversionRate, _convertedUnits, _displayText);

    }



  }

and UnitConverterBuilder as:

  public class UnitConverterBuilder : ConverterBuilder<UnitConverterBuilder>

  {

    public UnitConverterBuilder()

    {



    }



    public UnitConverterBuilder AddSomethingElse(object somethingElse)

    {

      // TODO: do something with somethingElse

      return this;

    }



  }

In this way your method chaining becomes straight forward and intuitive again:

  UnitConverterBuilder builder = new UnitConverterBuilder();

  IConverter converter = builder

    .AddConversionRate(2.54)

    .AddDisplayText("Inches to cm")

    .AddSomethingElse("something else")

    .Build();

But I think that allowing to extent a builder in this way violates the idea of the builder pattern, because a builder is supposed to implement a certain interface that is the contract between the builder and director. You don't relate to that part of the pattern at all?

You should be able to use the builder like this:

IBuilder builder = new UnitConverterBuilder(); //(or just: new ConverterBuilder();)

IConverter converter = builder.AddConversionRate(10).AddXXX()...Build();

where IBuilder is defined as:

public interface IBuilder 

{

  IBuilder AddConvertionRate(double rate);

  IBuilder AddXXX(...);

  ...

  IConverter Build();

}

Update:

As an answer to Matthwes question in his comment:

It's a step on the path: An interface is the abstraction or contract between the implementer and the client/consumer. So in principle your controller class should not know about the actual builder and converter implementations but fully rely on interfaces: IConverterBuilder which could be injected in the controllers constructor (in asp.net-core there is api's to do that from the startup class) and in the Convert(string text, double conversionRate) action it should call the apis (Addxxx(), Addyyy, Build()) of the injected builder interface to build the converter, but only know it as the contract: IConverter. In this way you implement both the Dependency Inversion and Dependency Injection principles.

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());