Two subtle Linq-related bugs that ReSharper highlighted during code review

ReSharper, a productivity tool that helps you automate some of the code writing and also analyses the code written. The biggest drawback I find with it is that it makes the Visual Studio startup a lot slower. Yes, that is annoying, but I still value the code quality help it gives me more than a fast startup.

This is two examples of Linq-related bugs that ReSharper pointed out for me when I was reviewing code. I hope I would have found the bugs anyway, but Resharper made it a no-brainer to find them and almost impossible to miss.
The examples do not show the original code, only the same problems.
The code writers did not use ReSharper.

Example 1

This is example 1, without the help of ReSharper, do you see the problem?

This is the same code, but with ReSharper active, the problem is shown more clearly, isn't it?

The gray code in the if-block tells us that the code isn't used. Ok, so why isn't it used? Hoover over the blue squiggly-lined code and you will see:

Expression is always false. This hopefully makes a programmer with little experience in Linq to do some research to find out why. And then change the if-condition

projects == null 

to 

projects.Count == 0 

or 

!projects.Any()

Because Linq's ToList() does never return null, if no rows are matched an empty list is returned.

Example 2

This is a similar problem, do you see what the problem is this time?

With ReSharper activated it looks like this again, but not for the same reason.

The variable projects is not the resulting list of the query. It IS the query. A query that hasn't been executed. And therefore, since it is a query (or an IQueryable object) that is assigned to a value during creation, it cannot be null when the execution of the code reaches the if-condition.

Conclusions

These two bugs can to a code writer or reviewer be rather subtle, but with the help of ReSharper they are not so subtle. Subtle bugs can easily go under the review radar and if the code isn't tested thoroughly it can even reach production. And bugs having come that far are often more cumbersome/expensive to fix than if they had been caught earlier in the process.
That's why I find ReSharper worth its price, both when it comes to money and performance.

C# Linq Equivalents in TypeScript

I have been looking for something like the C# Linq functions in TypeScript, but didn't have any luck. Today a colleague tipped about this site, where Linqs IEnumerable<T> extension methods are listed together with the methods TypeScript equivalent.

A few examples to give you an idea of what it can look like.

Refactoring: Extract Method example in C#

Many years ago I read the book Refactoring - Improving the design of existing code by Martin Fowler and found that it contained lots of interesting thoughts about code readability.

Instead of just doing a reread of it, I thought it might be interesting to write about the refactors that I've had the most use of during my time as a (mostly web) developer.

The examples in the book are written i Java, I will copy and translate them to C#.

I begin the series with the Extract Method refactoring.

Extract Method

You have a code fragment that can be grouped together.

Turn the fragment into a method whose name explains the purpose of the method.

The code before

The example is a short method that prints out how much a customer owes.

1:  public void PrintOwing()  
2:  {  
3:    Decimal outstanding = 0;  
4:    
5:    // print banner  
6:    Console.Out.WriteLine("*************************");  
7:    Console.Out.WriteLine("***** Customer Owes *****");  
8:    Console.Out.WriteLine("*************************");  
9:    
10:    // calculate outstanding  
11:    foreach (var order in _orders)  
12:    {  
13:      outstanding += order.GetAmount();  
14:    }  
15:    
16:    // print details  
17:    Console.Out.WriteLine("name: " + _name);  
18:    Console.Out.WriteLine("amount: " + outstanding);  
19:  }  

The code after

The three sections in PrintOwing has been moved out to three separate methods, which makes the PrintOwing method really short and you get the big picture of what it does real easy.

1:  public void PrintOwing()  
2:  {  
3:    PrintBanner();  
4:    var outstanding = GetOutstanding();  
5:    PrintDetails(outstanding);  
6:  }  
7:    
8:  private void PrintBanner()  
9:  {  
10:    Console.Out.WriteLine("*************************");  
11:    Console.Out.WriteLine("***** Customer Owes *****");  
12:    Console.Out.WriteLine("*************************");  
13:  }  
14:    
15:  private decimal GetOutstanding()  
16:  {  
17:    Decimal outstanding = 0;  
18:    foreach (var order in _orders)  
19:    {  
20:      outstanding += order.GetAmount();  
21:    }  
22:    return outstanding;  
23:  }  
24:    
25:  private void PrintDetails(decimal outstanding)  
26:  {  
27:    Console.Out.WriteLine("name: " + _name);  
28:    Console.Out.WriteLine("amount: " + outstanding);  
29:  }  

Motivation

The big wins with the extract method refactoring is that the code is divided into fine grained chunks that increases the chance of code reuse, and that the code itself can be read more like a series of comments.

How to Refactor

Read more about how to do this refactor step-by-step and more here.

The source code

https://github.com/carlbjorknas/RefactoringExamplesInCSharp.git

Personal thoughts

Replace foreach with Linq

I really like to Linq-ify code when it makes the code shorter and better expresses what it does. Like replacing a foreach loop with Linqs Sum method.

I prefer this

1:  private decimal GetOutstanding()  
2:  {  
3:    return _orders.Sum(order => order.GetAmount());  
4:  }  

instead of this

1:  private decimal GetOutstanding()  
2:  {  
3:    Decimal outstanding = 0;  
4:    foreach (var order in _orders)  
5:    {  
6:      outstanding += order.GetAmount();  
7:    }  
8:    return outstanding;  
9:  }  

because I think Sum shows more clearly than the foreach loop what we're after.

Thoughts that popped up during writing

The PrintOwing method was short to begin with. Did the three extract method operations really make it better? I've performed the extract method on much longer methods, where I clearly saw the advantage, but here I don't find it quite as clear.

One thing that happens when you extract code to smaller methods is that its makes the code easier to reuse. Suppose I want to print the same banner from another method, after the refactoring I can just call the PrintBanner method instead of copy-pasting the code.

So, what's the problem with copy-paste coding? Well, suppose you need to print the same banner in five different methods. If you haven't extracted the banner printing code to a separate method the same code will exist at five places. What happens when your customer wants you to change the banner layout, maybe change the "*" to a "#"? Will you find every copy of the banner printing code? With five different copies of it, I think the risk is high that some of the copies will be forgotten to be changed, and voila, you've created a bug!

My guess is that although PrintOwing was short from the start, it is possible to understand the big picture about what it does faster when it is even shorter. Compare these two really hypothetical eye movement scanning examples when reading the code to get the big picture of what PrintOwing does.

In the example above, you have to read a lot of code to understand the big picture of what the code does. That's bad, but on the other side, you get a good bit of insight into the details. You can see that the eyes make big jumps up and down when looking for a variable and what value it has been assigned to, and how it has been used. Details are mixed with the bigger picture.

In this second example the eyes have to scan a lot less code and don't have to do big jumps to find out where the variable gets its value from.

Maybe you would scan the code in the first example a lot different than I've drawn. Maybe you scan for comments, read them and skip the code. Well, I read comments as well, but my experience is that way too often the comment related to the code does not really match the code. The comment says one thing, and the code does something like it but not quite, or maybe even something waaay off.

In this example, the comments are replaced with the names of the methods the code is extracted to, so the problem can still exist, if the method is given a bad name it won't match the code it contains.

"ToLookup" to the rescue when "GroupBy" or "ToDictionary" isn't quite right for the job

More than once I've been writing code using LINQs GroupBy and ToDictionary methods with a feeling that the code didn't become as clear as I wanted it to be. I did some web searching, hoping that there might exist a third party library with a smart solution, but what I found though, was that I didn't even have to install another library, LINQ already contained a method for it!

I'll try to show you how ToLookup can be useful with a phony example.

Example

Suppose a group of smurfs take turn rolling a die and want us to print the result on the screen, like this:

 Smurfs that rolled a 1  
  - Grouchy Smurf  
  - Greedy Smurf  
   
 Smurfs that rolled a 2  
  - Smurfette  
   
 Smurfs that rolled a 3  
  - Papa Smurf  
  - Hefty Smurf  
   
 Smurfs that rolled a 4  
   
 Smurfs that rolled a 5  
  - Brainy Smurf  
   
 Smurfs that rolled a 6  
  - Clumsy Smurf  

For this I use a class to store each result in:

 class DieRollResult  
 {  
   public string Player { get; set; }  
   public int Value { get; set; }  
 }   

And a list to store all the results of a round in:

 var resultList = new List<DieRollResult>()  
 {  
   new DieRollResult{ Player= "Papa Smurf", Value = 3},  
   new DieRollResult{ Player= "Smurfette", Value = 2},  
   new DieRollResult{ Player= "Hefty Smurf", Value = 3},  
   new DieRollResult{ Player= "Brainy Smurf", Value = 5},  
   new DieRollResult{ Player= "Grouchy Smurf", Value = 1},  
   new DieRollResult{ Player= "Clumsy Smurf", Value = 6},  
   new DieRollResult{ Player= "Greedy Smurf", Value = 1},  
 };  

A solution using GroupBy

Notice the usage of SingleOrDefault to find the right group, it isn't possible to find it by using an index. 

Notice also that using this result set no smurf rolled a 4, and because empty groups like this are allowed a null check has to be done before iterating through the group to avoid a NullReferenceException.

A solution using GroupBy combined with ToDictionary

This time, notice the rather verbose expression to assign the variable resultGroups.

Notice also that this time, the group can be fetched using an index, but we still have to check if the dictionary really contains that index key to avoid a KeyNotFoundException.

A solution using ToLookup

So, finally, by using the ToLookup method, the assignment of the variable resultGroups is as short and clear as in the GroupBy example.

It is also possible to use an index to get the right group to iterate through. And since the ILookup object just returns an empty IEnumerable when it is indexed with a key that it doesn't contain, there is no need to check for null or if the key exists like we had to do in the other examples!

This is why I like the ToLookup method, it raises the signal-to-noise ratio a tiny bit, but sometimes it is just that small improvement that can make the code clearer. 

On the downside though, the method seems to be used so seldom that I think few programmers have seen it, so using it might make the code break the "Principle of least surprise".