Having a look at the Blazorise github source, I encountered this method:
protected override string FormatValueAsString( IReadOnlyList<TValue> value )
{
if ( value == null || value.Count == 0 )
return string.Empty;
if ( Multiple )
{
return string.Empty;
}
else
{
if ( value[0] == null )
return string.Empty;
return value[0].ToString();
}
}
Doesn't that seem like a lot of code for some simple behaviour ? I've seen development styles like this before, often with the argument that it's easy to read. I suppose that is true. But it also misses the point -- you are not using the c# language and libraries to their full extent...this does:
protected override string FormatValueAsString(IReadOnlyList<TValue> values) {
var result = values?.FirstOrDefault()?.ToString();
return result == null || Multiple ? string.Empty : result;
}
It's shorter, loses no meaning, uses LINQ acceptably and has one return statement instead of four. I also took the liberty of removing the strange spacing around the method argument and naming it more appropriately. And using the K&R statement styling. However, at the expense of some readability, but with a better performance profile, you could write:
protected override string FormatValueAsString(IReadOnlyList<TValue> values)
=> Multiple ? string.Empty : values?.FirstOrDefault()?.ToString() ?? string.Empty;
If I was being picky:
- I'd have a left a QA comment asking if the method name was really appropriate -- it's borderline IMO.
- The shorter implementations allow for the possibility that the ToString() method of TValue might return null (a defect in that case) - you can't discount that as a possibility, and it would possibly break the caller of the method
- An engineering approach might include a pre-condition that 'values' cannot be null
- A post condition would be that the return result is always not null
- The use of 'Multiple' looks a little forced - without delving further, could this test even be avoided altogether and be outside?
I'm very much a fan of internal as well as external quality.
No comments:
Post a Comment