Saturday, October 05, 2013

Stop doing this

Pet peeve: I hate micro-management when it comes to code. Yes, it's faster in the short-term but it's a pain when the code has to be changed. (If you don't know that all code has to be changed, you haven't programmed long enough.)

In this particular case I have encountered code similar to:

  var sb = new StringBuilder();
  foreach (var p in list)
  {
    sb.Append(p.Name);
    sb.Append(" ");
    sb.Append(p.Quantity);
    sb.Append("*");
    sb.Append(p.Price);
    sb.Append("=");
    sb.Append(p.Value);
    sb.Append(",");
  }
  if (list.Any())
  {
    sb.Remove(sb.Length - 1, 1);
  }
  
  return sb.ToString();

(It's even worse when it's a for loop instead of foreach, but let's not go there.)

The problem with this code is that it's extremely easy to get lost in the details. "Oh, I should put spaces around the operations." "Oh, I should add a space after the comma, but then it has to be removed at the end of the method." All of these are perfectly valid requests, but their place is not here. The purpose of this method is simple: generate a comma-separated list of products. That's all it does so that's all it should say:

  return string.Join(",", list.Select(ProductAsString));

(It's even better if you can use the ToString method instead of ProductAsString, but that's not always possible.)

The ProductAsString method is one abstraction level lower; it's fine for it to care about the details of creating a string representation of a product:

  private string ProductAsString(Product p)
  {
    var sb = new StringBuilder();
    sb.Append(p.Name);
    sb.Append(" ");
    sb.Append(p.Quantity);
    sb.Append("*");
    sb.Append(p.Price);
    sb.Append("=");
    sb.Append(p.Value);
    
    return sb.ToString();
  }

Of course, unless the speed is really much better with a StringBuilder instead of the string.Format method - and unless that's actually measured to be relevant - you should simplify the ProductAsString method to:

  private string ProductAsString(Product p)
  {
    return string.Format("{0} {1}*{2}={3}", p.Name, p.Quantity, p.Price, p.Value);
  }

This is much more expressive than the previous version.

Finally, the place of the ProductAsString method is among the method of the Product class. (It shows a severe case of feature envy.) Again, the best place would be the ToString method; if that's not possible for any reason, at least make it a public method of the Product class.

Ok, enough with the ranting, back to work.

3 comments:

Bertrand said...

Mmh, that is focusing entirely on code aesthetics, at the expense of performance. Instantiating a string builder, or doing a format inside of a loop are both bad ideas that can affect perf quite fast. Passing in the builder to use into the method would be a way around the problem. Also good to know: when there is a fixed number of strings to concatenate, good old concatenation is faster than both string builders and format. Format is the slowest of all.

Marcel said...

I agree with your comment on performance: yes, Format is the slowest (I was quite shocked to discover that). However, focusing entirely on code aesthetics (at the expense of everything else) is something that we need more of, in my opinion. Yes, performance is important - sometimes.

I have a project on GitHub - https://github.com/mdpopescu/public/tree/master/SimpleViewEngine - that uses reflection, dynamic variables and regular expressions to transform templates with {{name}} strings in them into HTML files. The performance is completely abysmal... and irrelevant when I'm waiting 8.5 seconds for the content server to return the images for the site I'm currently working on (true story [tm]).

I guess I don't disagree with you (ha! I'll write down the date!) but I'm a big believer in treating "make it fast" as much lower priority than "make it good (design)".

Thanks for commenting.

[That was a joke, btw; I don't normally disagree with you on programming issues. At least not so far. :P]

Bertrand said...

I don't disagree either ;) Performance is a layer, not what you base your design on. It's just that in this case, it probably deserves mentioning as it's very easy to degrade perf a lot doing this.