Saturday, January 23, 2021

Brevity or obfuscation in c#

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: