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 :)
.net design-patterns interface .net-core solid-principles
|
show 3 more comments
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 :)
.net design-patterns interface .net-core solid-principles
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
|
show 3 more comments
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 :)
.net design-patterns interface .net-core solid-principles
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
.net design-patterns interface .net-core solid-principles
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
|
show 3 more comments
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
|
show 3 more comments
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 inConverter.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.gvar 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 :)
add a comment |
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 inConverter.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.gvar 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 :)
add a comment |
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 inConverter.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.gvar 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 :)
add a comment |
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 inConverter.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.gvar 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 :)
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 inConverter.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.gvar 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 :)
answered Nov 13 at 9:23
Tom Halson
214
214
add a comment |
add a comment |
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.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
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
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