Solid Principles / Builder Pattern











up vote
-1
down vote

favorite












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










share|improve this question






















  • Hmmm, here I would just have a single static UnitConverter class with an enumeration of supported units (NOT Strings) and built-in conversion rates. Also, your IConverter interface would appear to violate SOLID with its DisplayText function.
    – Ryan Pierce Williams
    Nov 11 at 11:14










  • Thanks, on the display text violating SOLID, could you expand on that?
    – Matthew Stott
    Nov 11 at 11:32










  • What does the DisplayText have to do with the converters job of converting? It violates the Single Responsibility Principle - now you are making it responsible for the UI.
    – Ryan Pierce Williams
    Nov 11 at 11:33








  • 2




    Ahh yes very good point, so move display text to the class and remove from interface
    – Matthew Stott
    Nov 11 at 11:36






  • 2




    This question belongs on codereview.stackexchange.com.
    – jaco0646
    Nov 13 at 20:10















up vote
-1
down vote

favorite












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










share|improve this question






















  • Hmmm, here I would just have a single static UnitConverter class with an enumeration of supported units (NOT Strings) and built-in conversion rates. Also, your IConverter interface would appear to violate SOLID with its DisplayText function.
    – Ryan Pierce Williams
    Nov 11 at 11:14










  • Thanks, on the display text violating SOLID, could you expand on that?
    – Matthew Stott
    Nov 11 at 11:32










  • What does the DisplayText have to do with the converters job of converting? It violates the Single Responsibility Principle - now you are making it responsible for the UI.
    – Ryan Pierce Williams
    Nov 11 at 11:33








  • 2




    Ahh yes very good point, so move display text to the class and remove from interface
    – Matthew Stott
    Nov 11 at 11:36






  • 2




    This question belongs on codereview.stackexchange.com.
    – jaco0646
    Nov 13 at 20:10













up vote
-1
down vote

favorite









up vote
-1
down vote

favorite











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










share|improve this question













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







.net design-patterns interface .net-core solid-principles






share|improve this question













share|improve this question











share|improve this question




share|improve this question










asked Nov 11 at 10:20









Matthew Stott

5719




5719












  • Hmmm, here I would just have a single static UnitConverter class with an enumeration of supported units (NOT Strings) and built-in conversion rates. Also, your IConverter interface would appear to violate SOLID with its DisplayText function.
    – Ryan Pierce Williams
    Nov 11 at 11:14










  • Thanks, on the display text violating SOLID, could you expand on that?
    – Matthew Stott
    Nov 11 at 11:32










  • What does the DisplayText have to do with the converters job of converting? It violates the Single Responsibility Principle - now you are making it responsible for the UI.
    – Ryan Pierce Williams
    Nov 11 at 11:33








  • 2




    Ahh yes very good point, so move display text to the class and remove from interface
    – Matthew Stott
    Nov 11 at 11:36






  • 2




    This question belongs on codereview.stackexchange.com.
    – jaco0646
    Nov 13 at 20:10


















  • Hmmm, here I would just have a single static UnitConverter class with an enumeration of supported units (NOT Strings) and built-in conversion rates. Also, your IConverter interface would appear to violate SOLID with its DisplayText function.
    – Ryan Pierce Williams
    Nov 11 at 11:14










  • Thanks, on the display text violating SOLID, could you expand on that?
    – Matthew Stott
    Nov 11 at 11:32










  • What does the DisplayText have to do with the converters job of converting? It violates the Single Responsibility Principle - now you are making it responsible for the UI.
    – Ryan Pierce Williams
    Nov 11 at 11:33








  • 2




    Ahh yes very good point, so move display text to the class and remove from interface
    – Matthew Stott
    Nov 11 at 11:36






  • 2




    This question belongs on codereview.stackexchange.com.
    – jaco0646
    Nov 13 at 20:10
















Hmmm, here I would just have a single static UnitConverter class with an enumeration of supported units (NOT Strings) and built-in conversion rates. Also, your IConverter interface would appear to violate SOLID with its DisplayText function.
– Ryan Pierce Williams
Nov 11 at 11:14




Hmmm, here I would just have a single static UnitConverter class with an enumeration of supported units (NOT Strings) and built-in conversion rates. Also, your IConverter interface would appear to violate SOLID with its DisplayText function.
– Ryan Pierce Williams
Nov 11 at 11:14












Thanks, on the display text violating SOLID, could you expand on that?
– Matthew Stott
Nov 11 at 11:32




Thanks, on the display text violating SOLID, could you expand on that?
– Matthew Stott
Nov 11 at 11:32












What does the DisplayText have to do with the converters job of converting? It violates the Single Responsibility Principle - now you are making it responsible for the UI.
– Ryan Pierce Williams
Nov 11 at 11:33






What does the DisplayText have to do with the converters job of converting? It violates the Single Responsibility Principle - now you are making it responsible for the UI.
– Ryan Pierce Williams
Nov 11 at 11:33






2




2




Ahh yes very good point, so move display text to the class and remove from interface
– Matthew Stott
Nov 11 at 11:36




Ahh yes very good point, so move display text to the class and remove from interface
– Matthew Stott
Nov 11 at 11:36




2




2




This question belongs on codereview.stackexchange.com.
– jaco0646
Nov 13 at 20:10




This question belongs on codereview.stackexchange.com.
– jaco0646
Nov 13 at 20:10












1 Answer
1






active

oldest

votes

















up vote
0
down vote













These are not necessarily related to SOLID principle but here are a few things I would mention if I was reviewing this code at work




  • Excessive use of char '/n'. If you wanted to change from '/n' to '/r/n' you would have to change it 5 times in Converter.Convert. A better way to handle this would be to store this in a variable or perhaps allow it to be set via the constructor.

  • You can use var instead of explicitly stating the variable type e.g var d = double.Parse(input, CultureInfo.InvariantCulture) * ConversionRate;

  • Variable names, clean code should be easy to read so rather than using names like d and doubles use names that are easy for someone reading the code to understand

  • You don't need to specify the properties of Converter on the interface IConverter, this is because the interface only needs to expose the behaviors of the object and the properties are more of an implementation detail. Removing the properties will allow to you to have multiple objects that implement IConvert without being forced to have those specific properties (interface segregation)


I hope this helped :)






share|improve this answer





















    Your Answer






    StackExchange.ifUsing("editor", function () {
    StackExchange.using("externalEditor", function () {
    StackExchange.using("snippets", function () {
    StackExchange.snippets.init();
    });
    });
    }, "code-snippets");

    StackExchange.ready(function() {
    var channelOptions = {
    tags: "".split(" "),
    id: "1"
    };
    initTagRenderer("".split(" "), "".split(" "), channelOptions);

    StackExchange.using("externalEditor", function() {
    // Have to fire editor after snippets, if snippets enabled
    if (StackExchange.settings.snippets.snippetsEnabled) {
    StackExchange.using("snippets", function() {
    createEditor();
    });
    }
    else {
    createEditor();
    }
    });

    function createEditor() {
    StackExchange.prepareEditor({
    heartbeatType: 'answer',
    convertImagesToLinks: true,
    noModals: true,
    showLowRepImageUploadWarning: true,
    reputationToPostImages: 10,
    bindNavPrevention: true,
    postfix: "",
    imageUploader: {
    brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
    contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
    allowUrls: true
    },
    onDemand: true,
    discardSelector: ".discard-answer"
    ,immediatelyShowMarkdownHelp:true
    });


    }
    });














    draft saved

    draft discarded


















    StackExchange.ready(
    function () {
    StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fstackoverflow.com%2fquestions%2f53247767%2fsolid-principles-builder-pattern%23new-answer', 'question_page');
    }
    );

    Post as a guest















    Required, but never shown

























    1 Answer
    1






    active

    oldest

    votes








    1 Answer
    1






    active

    oldest

    votes









    active

    oldest

    votes






    active

    oldest

    votes








    up vote
    0
    down vote













    These are not necessarily related to SOLID principle but here are a few things I would mention if I was reviewing this code at work




    • Excessive use of char '/n'. If you wanted to change from '/n' to '/r/n' you would have to change it 5 times in Converter.Convert. A better way to handle this would be to store this in a variable or perhaps allow it to be set via the constructor.

    • You can use var instead of explicitly stating the variable type e.g var d = double.Parse(input, CultureInfo.InvariantCulture) * ConversionRate;

    • Variable names, clean code should be easy to read so rather than using names like d and doubles use names that are easy for someone reading the code to understand

    • You don't need to specify the properties of Converter on the interface IConverter, this is because the interface only needs to expose the behaviors of the object and the properties are more of an implementation detail. Removing the properties will allow to you to have multiple objects that implement IConvert without being forced to have those specific properties (interface segregation)


    I hope this helped :)






    share|improve this answer

























      up vote
      0
      down vote













      These are not necessarily related to SOLID principle but here are a few things I would mention if I was reviewing this code at work




      • Excessive use of char '/n'. If you wanted to change from '/n' to '/r/n' you would have to change it 5 times in Converter.Convert. A better way to handle this would be to store this in a variable or perhaps allow it to be set via the constructor.

      • You can use var instead of explicitly stating the variable type e.g var d = double.Parse(input, CultureInfo.InvariantCulture) * ConversionRate;

      • Variable names, clean code should be easy to read so rather than using names like d and doubles use names that are easy for someone reading the code to understand

      • You don't need to specify the properties of Converter on the interface IConverter, this is because the interface only needs to expose the behaviors of the object and the properties are more of an implementation detail. Removing the properties will allow to you to have multiple objects that implement IConvert without being forced to have those specific properties (interface segregation)


      I hope this helped :)






      share|improve this answer























        up vote
        0
        down vote










        up vote
        0
        down vote









        These are not necessarily related to SOLID principle but here are a few things I would mention if I was reviewing this code at work




        • Excessive use of char '/n'. If you wanted to change from '/n' to '/r/n' you would have to change it 5 times in Converter.Convert. A better way to handle this would be to store this in a variable or perhaps allow it to be set via the constructor.

        • You can use var instead of explicitly stating the variable type e.g var d = double.Parse(input, CultureInfo.InvariantCulture) * ConversionRate;

        • Variable names, clean code should be easy to read so rather than using names like d and doubles use names that are easy for someone reading the code to understand

        • You don't need to specify the properties of Converter on the interface IConverter, this is because the interface only needs to expose the behaviors of the object and the properties are more of an implementation detail. Removing the properties will allow to you to have multiple objects that implement IConvert without being forced to have those specific properties (interface segregation)


        I hope this helped :)






        share|improve this answer












        These are not necessarily related to SOLID principle but here are a few things I would mention if I was reviewing this code at work




        • Excessive use of char '/n'. If you wanted to change from '/n' to '/r/n' you would have to change it 5 times in Converter.Convert. A better way to handle this would be to store this in a variable or perhaps allow it to be set via the constructor.

        • You can use var instead of explicitly stating the variable type e.g var d = double.Parse(input, CultureInfo.InvariantCulture) * ConversionRate;

        • Variable names, clean code should be easy to read so rather than using names like d and doubles use names that are easy for someone reading the code to understand

        • You don't need to specify the properties of Converter on the interface IConverter, this is because the interface only needs to expose the behaviors of the object and the properties are more of an implementation detail. Removing the properties will allow to you to have multiple objects that implement IConvert without being forced to have those specific properties (interface segregation)


        I hope this helped :)







        share|improve this answer












        share|improve this answer



        share|improve this answer










        answered Nov 13 at 9:23









        Tom Halson

        214




        214






























            draft saved

            draft discarded




















































            Thanks for contributing an answer to Stack Overflow!


            • Please be sure to answer the question. Provide details and share your research!

            But avoid



            • Asking for help, clarification, or responding to other answers.

            • Making statements based on opinion; back them up with references or personal experience.


            To learn more, see our tips on writing great answers.





            Some of your past answers have not been well-received, and you're in danger of being blocked from answering.


            Please pay close attention to the following guidance:


            • Please be sure to answer the question. Provide details and share your research!

            But avoid



            • Asking for help, clarification, or responding to other answers.

            • Making statements based on opinion; back them up with references or personal experience.


            To learn more, see our tips on writing great answers.




            draft saved


            draft discarded














            StackExchange.ready(
            function () {
            StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fstackoverflow.com%2fquestions%2f53247767%2fsolid-principles-builder-pattern%23new-answer', 'question_page');
            }
            );

            Post as a guest















            Required, but never shown





















































            Required, but never shown














            Required, but never shown












            Required, but never shown







            Required, but never shown

































            Required, but never shown














            Required, but never shown












            Required, but never shown







            Required, but never shown







            Popular posts from this blog

            Full-time equivalent

            さくらももこ

            13 indicted, 8 arrested in Calif. drug cartel investigation