InputCommand Refactoring

Now we look at InputCommand. The first thing we notice is something we noticed the other day. Here s the code that concerns us:

 public InputCommand(StringReader reader) { 
 lines = new ArrayList(); 
 String line = reader.ReadLine(); 
 while (line != null && line != "*end") { 
 lines.Add(line.TrimEnd()); 
 line = reader.ReadLine(); 
 } 
 CleanTheLines(); 
 } 

The problem here is that the code is at two levels of abstraction. It does two things: first it reads the input into lines, and then it creates the clean lines, the lines with the vertical bar that represents the cursor position removed. But it does the first thing with a patch of code and the second with a method call.

The right structure for a method that does more than one thing is called Composed Method. Always create a method that is composed of nothing but method calls, all at the same level, rather than expanding any of the ideas out in line. So we did the Extract Method refactoring, to produce this result:

 public InputCommand(StringReader reader) { 
 ReadLines(reader); 
 CleanTheLines(); 
 } 
 private void ReadLines(StringReader reader) { 
 lines = new ArrayList(); 
 String line = reader.ReadLine(); 
 while (line != null && line != "*end") { 
 lines.Add(line.TrimEnd()); 
 line = reader.ReadLine(); 
 } 
 reader.Close; 
 } 

We added the reader.Close because it seems like a good idea to close a reader when you re done with it. As always, we compile, preparing to run the tests. Oops! Doesn t compile! We left the () off Close. We think it s a bug in C# to require parens on methods with no parameters, but Bill isn t here to complain to, so we fix the code to say Close(). That compiles fine, and we run the tests.

BANG! The tests fail. They re trying to read from a closed StringReader. Doh! We were passed this open StringReader. It s no business of ours to close it. This was a bum idea all around, so we remove the Close.

Would Watts Have Helped Us?

We stopped for a moment to chat about whether doing what Watts recommended would have helped us, since this was the first compile error and the first test error we had. Our answer is no, because we were entirely sure we wanted to do this thing. We did think about it: it was exactly what we were thinking about. Our thinking was wrong ”but our tests were not. Very interesting...

CleanTheLines

We don t like the method CleanTheLines. We don t usually use The as part of a method name. CleanLines would be a better name . We almost change it but then realize that the accessor to the clean lines from outside is called CleanLines. That s probably why we named the method so oddly to begin with. We try various ideas in our minds, like renaming the outside method to CleanedLines, and the like. None of the ideas appeal to us. What we do instead is remove the method altogether. We do that by moving the line-cleaning code directly into the CleanLines access method. The old method just returned the instance variable cleanLines:

 private ArrayList cleanLines; 
 public ArrayList CleanLines() { 
 return cleanLines; 
 } 

The new version eliminates the member variable and looks like this:

 public ArrayList CleanLines() { 
 ArrayList cleanLines = new ArrayList(); 
 foreach ( String line in lines) { 
 cleanLines.Add(CleanTheLine(line)); 
 } 
 return cleanLines; 
 } 

This compiles, and the tests run fine. The world is a slightly better place.