Length units converter
up vote
5
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 :)
c# object-oriented design-patterns asp.net-core
New contributor
|
show 2 more comments
up vote
5
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 :)
c# object-oriented design-patterns asp.net-core
New contributor
Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have.
– Toby Speight
Nov 14 at 10:32
Builder implementing abstract builder class - this one is empty and not abstract - did you forget to copy/paste something?
– t3chb0t
Nov 14 at 10:51
2
What’s your use-case? What benefit does having an interface and a builder class provide? For that matter, what benefit does having a class provide? Why not have a staticConvert
function? Lastly, I’m puzzled thatConvert
takes astring
and returnsstring
. It would be necessary to clarify this contract but I’d generally expect a signature of the formdouble Convert(double input)
or (as suggested)static double Convert(double input, double conversionRate)
. Such a method adheres much closer to SOLID than your implementation.
– Konrad Rudolph
Nov 14 at 17:25
1
@t3chb0t My suggested staticConvert
function deals perfectly well with dependency injection. There are tons of ways of implementing dependency injection, it doesn’t necessitate any of the boilerplate shown here.
– Konrad Rudolph
Nov 14 at 20:58
3
@t3chb0t Just pass a different method as a delegate. Or, better yet: don’t, because there’s no need to mock the conversion during testing. Instead, inject an appropriate conversion by passing aconversionRate
of your choice. That’s the only dependency here that is worth replacing. In other words: don’t blindly apply principles without first checking whether they’re being applied appropriately. Testing for its own sake is useless, it should be applied to solve specific problems.
– Konrad Rudolph
Nov 15 at 9:13
|
show 2 more comments
up vote
5
down vote
favorite
up vote
5
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 :)
c# object-oriented design-patterns asp.net-core
New contributor
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 :)
c# object-oriented design-patterns asp.net-core
c# object-oriented design-patterns asp.net-core
New contributor
New contributor
edited Nov 14 at 10:31
Toby Speight
21.9k536108
21.9k536108
New contributor
asked Nov 14 at 10:11
Matthew Stott
313
313
New contributor
New contributor
Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have.
– Toby Speight
Nov 14 at 10:32
Builder implementing abstract builder class - this one is empty and not abstract - did you forget to copy/paste something?
– t3chb0t
Nov 14 at 10:51
2
What’s your use-case? What benefit does having an interface and a builder class provide? For that matter, what benefit does having a class provide? Why not have a staticConvert
function? Lastly, I’m puzzled thatConvert
takes astring
and returnsstring
. It would be necessary to clarify this contract but I’d generally expect a signature of the formdouble Convert(double input)
or (as suggested)static double Convert(double input, double conversionRate)
. Such a method adheres much closer to SOLID than your implementation.
– Konrad Rudolph
Nov 14 at 17:25
1
@t3chb0t My suggested staticConvert
function deals perfectly well with dependency injection. There are tons of ways of implementing dependency injection, it doesn’t necessitate any of the boilerplate shown here.
– Konrad Rudolph
Nov 14 at 20:58
3
@t3chb0t Just pass a different method as a delegate. Or, better yet: don’t, because there’s no need to mock the conversion during testing. Instead, inject an appropriate conversion by passing aconversionRate
of your choice. That’s the only dependency here that is worth replacing. In other words: don’t blindly apply principles without first checking whether they’re being applied appropriately. Testing for its own sake is useless, it should be applied to solve specific problems.
– Konrad Rudolph
Nov 15 at 9:13
|
show 2 more comments
Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have.
– Toby Speight
Nov 14 at 10:32
Builder implementing abstract builder class - this one is empty and not abstract - did you forget to copy/paste something?
– t3chb0t
Nov 14 at 10:51
2
What’s your use-case? What benefit does having an interface and a builder class provide? For that matter, what benefit does having a class provide? Why not have a staticConvert
function? Lastly, I’m puzzled thatConvert
takes astring
and returnsstring
. It would be necessary to clarify this contract but I’d generally expect a signature of the formdouble Convert(double input)
or (as suggested)static double Convert(double input, double conversionRate)
. Such a method adheres much closer to SOLID than your implementation.
– Konrad Rudolph
Nov 14 at 17:25
1
@t3chb0t My suggested staticConvert
function deals perfectly well with dependency injection. There are tons of ways of implementing dependency injection, it doesn’t necessitate any of the boilerplate shown here.
– Konrad Rudolph
Nov 14 at 20:58
3
@t3chb0t Just pass a different method as a delegate. Or, better yet: don’t, because there’s no need to mock the conversion during testing. Instead, inject an appropriate conversion by passing aconversionRate
of your choice. That’s the only dependency here that is worth replacing. In other words: don’t blindly apply principles without first checking whether they’re being applied appropriately. Testing for its own sake is useless, it should be applied to solve specific problems.
– Konrad Rudolph
Nov 15 at 9:13
Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have.
– Toby Speight
Nov 14 at 10:32
Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have.
– Toby Speight
Nov 14 at 10:32
Builder implementing abstract builder class - this one is empty and not abstract - did you forget to copy/paste something?
– t3chb0t
Nov 14 at 10:51
Builder implementing abstract builder class - this one is empty and not abstract - did you forget to copy/paste something?
– t3chb0t
Nov 14 at 10:51
2
2
What’s your use-case? What benefit does having an interface and a builder class provide? For that matter, what benefit does having a class provide? Why not have a static
Convert
function? Lastly, I’m puzzled that Convert
takes a string
and returns string
. It would be necessary to clarify this contract but I’d generally expect a signature of the form double Convert(double input)
or (as suggested) static double Convert(double input, double conversionRate)
. Such a method adheres much closer to SOLID than your implementation.– Konrad Rudolph
Nov 14 at 17:25
What’s your use-case? What benefit does having an interface and a builder class provide? For that matter, what benefit does having a class provide? Why not have a static
Convert
function? Lastly, I’m puzzled that Convert
takes a string
and returns string
. It would be necessary to clarify this contract but I’d generally expect a signature of the form double Convert(double input)
or (as suggested) static double Convert(double input, double conversionRate)
. Such a method adheres much closer to SOLID than your implementation.– Konrad Rudolph
Nov 14 at 17:25
1
1
@t3chb0t My suggested static
Convert
function deals perfectly well with dependency injection. There are tons of ways of implementing dependency injection, it doesn’t necessitate any of the boilerplate shown here.– Konrad Rudolph
Nov 14 at 20:58
@t3chb0t My suggested static
Convert
function deals perfectly well with dependency injection. There are tons of ways of implementing dependency injection, it doesn’t necessitate any of the boilerplate shown here.– Konrad Rudolph
Nov 14 at 20:58
3
3
@t3chb0t Just pass a different method as a delegate. Or, better yet: don’t, because there’s no need to mock the conversion during testing. Instead, inject an appropriate conversion by passing a
conversionRate
of your choice. That’s the only dependency here that is worth replacing. In other words: don’t blindly apply principles without first checking whether they’re being applied appropriately. Testing for its own sake is useless, it should be applied to solve specific problems.– Konrad Rudolph
Nov 15 at 9:13
@t3chb0t Just pass a different method as a delegate. Or, better yet: don’t, because there’s no need to mock the conversion during testing. Instead, inject an appropriate conversion by passing a
conversionRate
of your choice. That’s the only dependency here that is worth replacing. In other words: don’t blindly apply principles without first checking whether they’re being applied appropriately. Testing for its own sake is useless, it should be applied to solve specific problems.– Konrad Rudolph
Nov 15 at 9:13
|
show 2 more comments
3 Answers
3
active
oldest
votes
up vote
6
down vote
accepted
The S in SOLID
Since you mention SOLID, I'd like to point out that IConverter
has too many responsibilities:
- It converts from one unit to another.
- It splits input on newlines
- 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 ofConvert
is an array of strings, among other things. - The builder contains two
AddConversionRate
methods - one of them should be renamed toAddConvertedUnits
. - 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 callingconverter.Convert
. - A related issue is that of code duplication: in
Convert
, theif
part is doing the same as theelse
part, minus the string-splitting. However, if the input does not contain any newlines thenstring.Split
will return an array with one element, so theelse
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}'";
}));
In your final comment where you hard-code 'inch and 'cm' into the getunit method how would you get these values from the ui? E.g. a from unit and a to unit. What I mean is this data would need to come from the ui and then be passed to a data access layer (if a db was used)
– Matthew Stott
Nov 15 at 19:03
I'd show a list of available units to the user (henceAvailableUnits
) and pass the chosen unit names back into the controller'sConvert
method via parameters:public IActionResult Convert(string input, string fromUnitName, string toUnitName)
.
– Pieter Witvoet
Nov 15 at 20:13
Thank you!! In terms of the factory pattern to get the data (with hard coded values lets say) would you simply create a factory class that returned a unit object based on the name provided. Isn't the factory pattern normally concerned with creation rather than getting or am I wrong
– Matthew Stott
Nov 15 at 20:23
If you only need units in this particular controller then aGetUnit
andAvailableUnits
method in the controller itself is sufficient. If other controllers also need to work with units then you'd move those methods to a shared model. Just because this approach matches the factory pattern (depending on how you view things) doesn't mean you need a separateFactory
class, or that this method should be namedUnitFactory
.
– Pieter Witvoet
Nov 16 at 14:15
add a comment |
up vote
10
down vote
Only targeting public string Convert(string input)
- Because this is a
public
method you should validate the input. Right now if one passesnull
this will throw anArgumentNullException
which isn't that bad but the stacktrace belongs toSystem.Linq.Enumerable.Contains()
which you should hide.
Just testing againstnull
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, UsingCount()
use theLength
property instead. - You should use the overloaded
Split()
method which takes a[StringSplitOptions][2]
enum likeStringSplitOptions.RemoveEmptyEntries
. - Instead of using
double.Parse()
you should consider to usedouble.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 anIEnumerable<string
or better just anIEnumerable<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 forn
.
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()
atfor (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());
3
Count()
Avoids iteration if the type isICollection
orICollection<T>
. Array implements the former so it won't be fully iterated. See reference source
– RobH
Nov 14 at 11:44
@RobH I know, but at least a softcast with a null check will be avoided.
– Heslacher
Nov 14 at 11:47
Thank you for comments will review asap
– Matthew Stott
Nov 14 at 12:19
@Heslacher What is a softcast? Don't think I've ever seen that terminology in any language specification ever. There is a virtual call involved certainly which might have marginal overhead (although HotSpot would optimise it away, not sure how good RyuJit is these days), but nothing is casted there as far as I can see. As it stands the claim is completely wrong and will make people worry about performance for no reason (there's a big difference between a virtual call and iterating through a possibly gigantic array).
– Voo
Nov 14 at 16:51
(Otherwise I completely agree with this answer, but this is such a big problem in my eyes that I had to downvote it, until it's fixed).
– Voo
Nov 14 at 16:57
|
show 1 more comment
up vote
9
down vote
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 are apis 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.
Henrik- so in your first comment regarding returning IConverter instead of Converter is this so we adhere to dependency inversion in that we should depend on abstractions not concrete
– Matthew Stott
Nov 14 at 20:52
@MatthewStott: see my update of the answer :-)
– Henrik Hansen
Nov 15 at 7:11
add a comment |
3 Answers
3
active
oldest
votes
3 Answers
3
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
6
down vote
accepted
The S in SOLID
Since you mention SOLID, I'd like to point out that IConverter
has too many responsibilities:
- It converts from one unit to another.
- It splits input on newlines
- 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 ofConvert
is an array of strings, among other things. - The builder contains two
AddConversionRate
methods - one of them should be renamed toAddConvertedUnits
. - 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 callingconverter.Convert
. - A related issue is that of code duplication: in
Convert
, theif
part is doing the same as theelse
part, minus the string-splitting. However, if the input does not contain any newlines thenstring.Split
will return an array with one element, so theelse
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}'";
}));
In your final comment where you hard-code 'inch and 'cm' into the getunit method how would you get these values from the ui? E.g. a from unit and a to unit. What I mean is this data would need to come from the ui and then be passed to a data access layer (if a db was used)
– Matthew Stott
Nov 15 at 19:03
I'd show a list of available units to the user (henceAvailableUnits
) and pass the chosen unit names back into the controller'sConvert
method via parameters:public IActionResult Convert(string input, string fromUnitName, string toUnitName)
.
– Pieter Witvoet
Nov 15 at 20:13
Thank you!! In terms of the factory pattern to get the data (with hard coded values lets say) would you simply create a factory class that returned a unit object based on the name provided. Isn't the factory pattern normally concerned with creation rather than getting or am I wrong
– Matthew Stott
Nov 15 at 20:23
If you only need units in this particular controller then aGetUnit
andAvailableUnits
method in the controller itself is sufficient. If other controllers also need to work with units then you'd move those methods to a shared model. Just because this approach matches the factory pattern (depending on how you view things) doesn't mean you need a separateFactory
class, or that this method should be namedUnitFactory
.
– Pieter Witvoet
Nov 16 at 14:15
add a comment |
up vote
6
down vote
accepted
The S in SOLID
Since you mention SOLID, I'd like to point out that IConverter
has too many responsibilities:
- It converts from one unit to another.
- It splits input on newlines
- 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 ofConvert
is an array of strings, among other things. - The builder contains two
AddConversionRate
methods - one of them should be renamed toAddConvertedUnits
. - 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 callingconverter.Convert
. - A related issue is that of code duplication: in
Convert
, theif
part is doing the same as theelse
part, minus the string-splitting. However, if the input does not contain any newlines thenstring.Split
will return an array with one element, so theelse
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}'";
}));
In your final comment where you hard-code 'inch and 'cm' into the getunit method how would you get these values from the ui? E.g. a from unit and a to unit. What I mean is this data would need to come from the ui and then be passed to a data access layer (if a db was used)
– Matthew Stott
Nov 15 at 19:03
I'd show a list of available units to the user (henceAvailableUnits
) and pass the chosen unit names back into the controller'sConvert
method via parameters:public IActionResult Convert(string input, string fromUnitName, string toUnitName)
.
– Pieter Witvoet
Nov 15 at 20:13
Thank you!! In terms of the factory pattern to get the data (with hard coded values lets say) would you simply create a factory class that returned a unit object based on the name provided. Isn't the factory pattern normally concerned with creation rather than getting or am I wrong
– Matthew Stott
Nov 15 at 20:23
If you only need units in this particular controller then aGetUnit
andAvailableUnits
method in the controller itself is sufficient. If other controllers also need to work with units then you'd move those methods to a shared model. Just because this approach matches the factory pattern (depending on how you view things) doesn't mean you need a separateFactory
class, or that this method should be namedUnitFactory
.
– Pieter Witvoet
Nov 16 at 14:15
add a comment |
up vote
6
down vote
accepted
up vote
6
down vote
accepted
The S in SOLID
Since you mention SOLID, I'd like to point out that IConverter
has too many responsibilities:
- It converts from one unit to another.
- It splits input on newlines
- 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 ofConvert
is an array of strings, among other things. - The builder contains two
AddConversionRate
methods - one of them should be renamed toAddConvertedUnits
. - 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 callingconverter.Convert
. - A related issue is that of code duplication: in
Convert
, theif
part is doing the same as theelse
part, minus the string-splitting. However, if the input does not contain any newlines thenstring.Split
will return an array with one element, so theelse
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}'";
}));
The S in SOLID
Since you mention SOLID, I'd like to point out that IConverter
has too many responsibilities:
- It converts from one unit to another.
- It splits input on newlines
- 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 ofConvert
is an array of strings, among other things. - The builder contains two
AddConversionRate
methods - one of them should be renamed toAddConvertedUnits
. - 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 callingconverter.Convert
. - A related issue is that of code duplication: in
Convert
, theif
part is doing the same as theelse
part, minus the string-splitting. However, if the input does not contain any newlines thenstring.Split
will return an array with one element, so theelse
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}'";
}));
edited Nov 15 at 12:34
answered Nov 15 at 11:07
Pieter Witvoet
4,721723
4,721723
In your final comment where you hard-code 'inch and 'cm' into the getunit method how would you get these values from the ui? E.g. a from unit and a to unit. What I mean is this data would need to come from the ui and then be passed to a data access layer (if a db was used)
– Matthew Stott
Nov 15 at 19:03
I'd show a list of available units to the user (henceAvailableUnits
) and pass the chosen unit names back into the controller'sConvert
method via parameters:public IActionResult Convert(string input, string fromUnitName, string toUnitName)
.
– Pieter Witvoet
Nov 15 at 20:13
Thank you!! In terms of the factory pattern to get the data (with hard coded values lets say) would you simply create a factory class that returned a unit object based on the name provided. Isn't the factory pattern normally concerned with creation rather than getting or am I wrong
– Matthew Stott
Nov 15 at 20:23
If you only need units in this particular controller then aGetUnit
andAvailableUnits
method in the controller itself is sufficient. If other controllers also need to work with units then you'd move those methods to a shared model. Just because this approach matches the factory pattern (depending on how you view things) doesn't mean you need a separateFactory
class, or that this method should be namedUnitFactory
.
– Pieter Witvoet
Nov 16 at 14:15
add a comment |
In your final comment where you hard-code 'inch and 'cm' into the getunit method how would you get these values from the ui? E.g. a from unit and a to unit. What I mean is this data would need to come from the ui and then be passed to a data access layer (if a db was used)
– Matthew Stott
Nov 15 at 19:03
I'd show a list of available units to the user (henceAvailableUnits
) and pass the chosen unit names back into the controller'sConvert
method via parameters:public IActionResult Convert(string input, string fromUnitName, string toUnitName)
.
– Pieter Witvoet
Nov 15 at 20:13
Thank you!! In terms of the factory pattern to get the data (with hard coded values lets say) would you simply create a factory class that returned a unit object based on the name provided. Isn't the factory pattern normally concerned with creation rather than getting or am I wrong
– Matthew Stott
Nov 15 at 20:23
If you only need units in this particular controller then aGetUnit
andAvailableUnits
method in the controller itself is sufficient. If other controllers also need to work with units then you'd move those methods to a shared model. Just because this approach matches the factory pattern (depending on how you view things) doesn't mean you need a separateFactory
class, or that this method should be namedUnitFactory
.
– Pieter Witvoet
Nov 16 at 14:15
In your final comment where you hard-code 'inch and 'cm' into the getunit method how would you get these values from the ui? E.g. a from unit and a to unit. What I mean is this data would need to come from the ui and then be passed to a data access layer (if a db was used)
– Matthew Stott
Nov 15 at 19:03
In your final comment where you hard-code 'inch and 'cm' into the getunit method how would you get these values from the ui? E.g. a from unit and a to unit. What I mean is this data would need to come from the ui and then be passed to a data access layer (if a db was used)
– Matthew Stott
Nov 15 at 19:03
I'd show a list of available units to the user (hence
AvailableUnits
) and pass the chosen unit names back into the controller's Convert
method via parameters: public IActionResult Convert(string input, string fromUnitName, string toUnitName)
.– Pieter Witvoet
Nov 15 at 20:13
I'd show a list of available units to the user (hence
AvailableUnits
) and pass the chosen unit names back into the controller's Convert
method via parameters: public IActionResult Convert(string input, string fromUnitName, string toUnitName)
.– Pieter Witvoet
Nov 15 at 20:13
Thank you!! In terms of the factory pattern to get the data (with hard coded values lets say) would you simply create a factory class that returned a unit object based on the name provided. Isn't the factory pattern normally concerned with creation rather than getting or am I wrong
– Matthew Stott
Nov 15 at 20:23
Thank you!! In terms of the factory pattern to get the data (with hard coded values lets say) would you simply create a factory class that returned a unit object based on the name provided. Isn't the factory pattern normally concerned with creation rather than getting or am I wrong
– Matthew Stott
Nov 15 at 20:23
If you only need units in this particular controller then a
GetUnit
and AvailableUnits
method in the controller itself is sufficient. If other controllers also need to work with units then you'd move those methods to a shared model. Just because this approach matches the factory pattern (depending on how you view things) doesn't mean you need a separate Factory
class, or that this method should be named UnitFactory
.– Pieter Witvoet
Nov 16 at 14:15
If you only need units in this particular controller then a
GetUnit
and AvailableUnits
method in the controller itself is sufficient. If other controllers also need to work with units then you'd move those methods to a shared model. Just because this approach matches the factory pattern (depending on how you view things) doesn't mean you need a separate Factory
class, or that this method should be named UnitFactory
.– Pieter Witvoet
Nov 16 at 14:15
add a comment |
up vote
10
down vote
Only targeting public string Convert(string input)
- Because this is a
public
method you should validate the input. Right now if one passesnull
this will throw anArgumentNullException
which isn't that bad but the stacktrace belongs toSystem.Linq.Enumerable.Contains()
which you should hide.
Just testing againstnull
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, UsingCount()
use theLength
property instead. - You should use the overloaded
Split()
method which takes a[StringSplitOptions][2]
enum likeStringSplitOptions.RemoveEmptyEntries
. - Instead of using
double.Parse()
you should consider to usedouble.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 anIEnumerable<string
or better just anIEnumerable<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 forn
.
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()
atfor (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());
3
Count()
Avoids iteration if the type isICollection
orICollection<T>
. Array implements the former so it won't be fully iterated. See reference source
– RobH
Nov 14 at 11:44
@RobH I know, but at least a softcast with a null check will be avoided.
– Heslacher
Nov 14 at 11:47
Thank you for comments will review asap
– Matthew Stott
Nov 14 at 12:19
@Heslacher What is a softcast? Don't think I've ever seen that terminology in any language specification ever. There is a virtual call involved certainly which might have marginal overhead (although HotSpot would optimise it away, not sure how good RyuJit is these days), but nothing is casted there as far as I can see. As it stands the claim is completely wrong and will make people worry about performance for no reason (there's a big difference between a virtual call and iterating through a possibly gigantic array).
– Voo
Nov 14 at 16:51
(Otherwise I completely agree with this answer, but this is such a big problem in my eyes that I had to downvote it, until it's fixed).
– Voo
Nov 14 at 16:57
|
show 1 more comment
up vote
10
down vote
Only targeting public string Convert(string input)
- Because this is a
public
method you should validate the input. Right now if one passesnull
this will throw anArgumentNullException
which isn't that bad but the stacktrace belongs toSystem.Linq.Enumerable.Contains()
which you should hide.
Just testing againstnull
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, UsingCount()
use theLength
property instead. - You should use the overloaded
Split()
method which takes a[StringSplitOptions][2]
enum likeStringSplitOptions.RemoveEmptyEntries
. - Instead of using
double.Parse()
you should consider to usedouble.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 anIEnumerable<string
or better just anIEnumerable<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 forn
.
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()
atfor (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());
3
Count()
Avoids iteration if the type isICollection
orICollection<T>
. Array implements the former so it won't be fully iterated. See reference source
– RobH
Nov 14 at 11:44
@RobH I know, but at least a softcast with a null check will be avoided.
– Heslacher
Nov 14 at 11:47
Thank you for comments will review asap
– Matthew Stott
Nov 14 at 12:19
@Heslacher What is a softcast? Don't think I've ever seen that terminology in any language specification ever. There is a virtual call involved certainly which might have marginal overhead (although HotSpot would optimise it away, not sure how good RyuJit is these days), but nothing is casted there as far as I can see. As it stands the claim is completely wrong and will make people worry about performance for no reason (there's a big difference between a virtual call and iterating through a possibly gigantic array).
– Voo
Nov 14 at 16:51
(Otherwise I completely agree with this answer, but this is such a big problem in my eyes that I had to downvote it, until it's fixed).
– Voo
Nov 14 at 16:57
|
show 1 more comment
up vote
10
down vote
up vote
10
down vote
Only targeting public string Convert(string input)
- Because this is a
public
method you should validate the input. Right now if one passesnull
this will throw anArgumentNullException
which isn't that bad but the stacktrace belongs toSystem.Linq.Enumerable.Contains()
which you should hide.
Just testing againstnull
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, UsingCount()
use theLength
property instead. - You should use the overloaded
Split()
method which takes a[StringSplitOptions][2]
enum likeStringSplitOptions.RemoveEmptyEntries
. - Instead of using
double.Parse()
you should consider to usedouble.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 anIEnumerable<string
or better just anIEnumerable<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 forn
.
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()
atfor (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());
Only targeting public string Convert(string input)
- Because this is a
public
method you should validate the input. Right now if one passesnull
this will throw anArgumentNullException
which isn't that bad but the stacktrace belongs toSystem.Linq.Enumerable.Contains()
which you should hide.
Just testing againstnull
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, UsingCount()
use theLength
property instead. - You should use the overloaded
Split()
method which takes a[StringSplitOptions][2]
enum likeStringSplitOptions.RemoveEmptyEntries
. - Instead of using
double.Parse()
you should consider to usedouble.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 anIEnumerable<string
or better just anIEnumerable<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 forn
.
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()
atfor (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());
edited Nov 14 at 17:03
answered Nov 14 at 10:47
Heslacher
44.5k460154
44.5k460154
3
Count()
Avoids iteration if the type isICollection
orICollection<T>
. Array implements the former so it won't be fully iterated. See reference source
– RobH
Nov 14 at 11:44
@RobH I know, but at least a softcast with a null check will be avoided.
– Heslacher
Nov 14 at 11:47
Thank you for comments will review asap
– Matthew Stott
Nov 14 at 12:19
@Heslacher What is a softcast? Don't think I've ever seen that terminology in any language specification ever. There is a virtual call involved certainly which might have marginal overhead (although HotSpot would optimise it away, not sure how good RyuJit is these days), but nothing is casted there as far as I can see. As it stands the claim is completely wrong and will make people worry about performance for no reason (there's a big difference between a virtual call and iterating through a possibly gigantic array).
– Voo
Nov 14 at 16:51
(Otherwise I completely agree with this answer, but this is such a big problem in my eyes that I had to downvote it, until it's fixed).
– Voo
Nov 14 at 16:57
|
show 1 more comment
3
Count()
Avoids iteration if the type isICollection
orICollection<T>
. Array implements the former so it won't be fully iterated. See reference source
– RobH
Nov 14 at 11:44
@RobH I know, but at least a softcast with a null check will be avoided.
– Heslacher
Nov 14 at 11:47
Thank you for comments will review asap
– Matthew Stott
Nov 14 at 12:19
@Heslacher What is a softcast? Don't think I've ever seen that terminology in any language specification ever. There is a virtual call involved certainly which might have marginal overhead (although HotSpot would optimise it away, not sure how good RyuJit is these days), but nothing is casted there as far as I can see. As it stands the claim is completely wrong and will make people worry about performance for no reason (there's a big difference between a virtual call and iterating through a possibly gigantic array).
– Voo
Nov 14 at 16:51
(Otherwise I completely agree with this answer, but this is such a big problem in my eyes that I had to downvote it, until it's fixed).
– Voo
Nov 14 at 16:57
3
3
Count()
Avoids iteration if the type is ICollection
or ICollection<T>
. Array implements the former so it won't be fully iterated. See reference source– RobH
Nov 14 at 11:44
Count()
Avoids iteration if the type is ICollection
or ICollection<T>
. Array implements the former so it won't be fully iterated. See reference source– RobH
Nov 14 at 11:44
@RobH I know, but at least a softcast with a null check will be avoided.
– Heslacher
Nov 14 at 11:47
@RobH I know, but at least a softcast with a null check will be avoided.
– Heslacher
Nov 14 at 11:47
Thank you for comments will review asap
– Matthew Stott
Nov 14 at 12:19
Thank you for comments will review asap
– Matthew Stott
Nov 14 at 12:19
@Heslacher What is a softcast? Don't think I've ever seen that terminology in any language specification ever. There is a virtual call involved certainly which might have marginal overhead (although HotSpot would optimise it away, not sure how good RyuJit is these days), but nothing is casted there as far as I can see. As it stands the claim is completely wrong and will make people worry about performance for no reason (there's a big difference between a virtual call and iterating through a possibly gigantic array).
– Voo
Nov 14 at 16:51
@Heslacher What is a softcast? Don't think I've ever seen that terminology in any language specification ever. There is a virtual call involved certainly which might have marginal overhead (although HotSpot would optimise it away, not sure how good RyuJit is these days), but nothing is casted there as far as I can see. As it stands the claim is completely wrong and will make people worry about performance for no reason (there's a big difference between a virtual call and iterating through a possibly gigantic array).
– Voo
Nov 14 at 16:51
(Otherwise I completely agree with this answer, but this is such a big problem in my eyes that I had to downvote it, until it's fixed).
– Voo
Nov 14 at 16:57
(Otherwise I completely agree with this answer, but this is such a big problem in my eyes that I had to downvote it, until it's fixed).
– Voo
Nov 14 at 16:57
|
show 1 more comment
up vote
9
down vote
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 are apis 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.
Henrik- so in your first comment regarding returning IConverter instead of Converter is this so we adhere to dependency inversion in that we should depend on abstractions not concrete
– Matthew Stott
Nov 14 at 20:52
@MatthewStott: see my update of the answer :-)
– Henrik Hansen
Nov 15 at 7:11
add a comment |
up vote
9
down vote
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 are apis 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.
Henrik- so in your first comment regarding returning IConverter instead of Converter is this so we adhere to dependency inversion in that we should depend on abstractions not concrete
– Matthew Stott
Nov 14 at 20:52
@MatthewStott: see my update of the answer :-)
– Henrik Hansen
Nov 15 at 7:11
add a comment |
up vote
9
down vote
up vote
9
down vote
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 are apis 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.
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 are apis 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.
edited Nov 15 at 14:26
answered Nov 14 at 16:14
Henrik Hansen
6,0181722
6,0181722
Henrik- so in your first comment regarding returning IConverter instead of Converter is this so we adhere to dependency inversion in that we should depend on abstractions not concrete
– Matthew Stott
Nov 14 at 20:52
@MatthewStott: see my update of the answer :-)
– Henrik Hansen
Nov 15 at 7:11
add a comment |
Henrik- so in your first comment regarding returning IConverter instead of Converter is this so we adhere to dependency inversion in that we should depend on abstractions not concrete
– Matthew Stott
Nov 14 at 20:52
@MatthewStott: see my update of the answer :-)
– Henrik Hansen
Nov 15 at 7:11
Henrik- so in your first comment regarding returning IConverter instead of Converter is this so we adhere to dependency inversion in that we should depend on abstractions not concrete
– Matthew Stott
Nov 14 at 20:52
Henrik- so in your first comment regarding returning IConverter instead of Converter is this so we adhere to dependency inversion in that we should depend on abstractions not concrete
– Matthew Stott
Nov 14 at 20:52
@MatthewStott: see my update of the answer :-)
– Henrik Hansen
Nov 15 at 7:11
@MatthewStott: see my update of the answer :-)
– Henrik Hansen
Nov 15 at 7:11
add a comment |
Matthew Stott is a new contributor. Be nice, and check out our Code of Conduct.
Matthew Stott is a new contributor. Be nice, and check out our Code of Conduct.
Matthew Stott is a new contributor. Be nice, and check out our Code of Conduct.
Matthew Stott is a new contributor. Be nice, and check out our Code of Conduct.
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%2fcodereview.stackexchange.com%2fquestions%2f207640%2flength-units-converter%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
Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have.
– Toby Speight
Nov 14 at 10:32
Builder implementing abstract builder class - this one is empty and not abstract - did you forget to copy/paste something?
– t3chb0t
Nov 14 at 10:51
2
What’s your use-case? What benefit does having an interface and a builder class provide? For that matter, what benefit does having a class provide? Why not have a static
Convert
function? Lastly, I’m puzzled thatConvert
takes astring
and returnsstring
. It would be necessary to clarify this contract but I’d generally expect a signature of the formdouble Convert(double input)
or (as suggested)static double Convert(double input, double conversionRate)
. Such a method adheres much closer to SOLID than your implementation.– Konrad Rudolph
Nov 14 at 17:25
1
@t3chb0t My suggested static
Convert
function deals perfectly well with dependency injection. There are tons of ways of implementing dependency injection, it doesn’t necessitate any of the boilerplate shown here.– Konrad Rudolph
Nov 14 at 20:58
3
@t3chb0t Just pass a different method as a delegate. Or, better yet: don’t, because there’s no need to mock the conversion during testing. Instead, inject an appropriate conversion by passing a
conversionRate
of your choice. That’s the only dependency here that is worth replacing. In other words: don’t blindly apply principles without first checking whether they’re being applied appropriately. Testing for its own sake is useless, it should be applied to solve specific problems.– Konrad Rudolph
Nov 15 at 9:13