What’s wrong with that: Do not only code the happy case

Last pull request I’ve opened was some kind of disappointing. When going through the code files, I detected a lot of… questions marks above my head.

We do use pull requests for two reasons actually:

• The need for doing review sessions goes down dramatically
• The team learns to improve each other while not being upset when anthing is not “good enough”

So here we are, some changes to the setup, thus no Dependency Injection in place. Actually I do like to have it in place pretty much everywhere but in case of setup, the overhead is pretty high. Most of the code does not allow for factories, components are “just” called. Nevertheless, would be great to have it in.

Anyway it is important to keep the rules of abstraction, sensible encapsulation and separation. Let’s have a look here and come up with some questions:

What does the name “GetNewestVersionNumber” tell?

Is there any method available that returns an old or current one? I wouldn’t find too many reason for doing so, but if the word “newest” is added to this method name, it should have a reason because it raises expectations on my side. So either remove it and stay with method names being verbs or verb phrases. Let’s say in a class that shall process actions, it could look like this:

interface ICommand<T> Where T: CommandContext
{
    void Execute(T);
    void CanExecute(T);
}

Don’t going into too much details about CommandContext here. You may want to have a look at how commands can be run seemlessly when using Windsor Castle/ DI and object models. The Execute method is just a verb. That does make it pretty easy to understand what’s going on. The name of the class/ command shall indicate clearly what is about to be executed, while the method name itself shall neither be redundant nor being not specific enough.

Let’s take another example:

 public interface ICommandExecutor
 {
 bool Execute(CommandContext context);
 bool Execute(List<CommandContext> contexts);

 bool Execute(List<CommandContext> prepareContexts, List<CommandContext> processContexts,
 List<CommandContext> finishContexts);
 }

We keep going with commands at this place. ICommandExecutor is certainly implemented by a class called CommandExecutor – also here following the best practices that the default implementation of an interface follows the same name, removing the prefix “I”. Actually there are three methods, one for executing just one command, the other two for executing multiple commands.

Not really surprisingly, the methods are called Execute, being available as overloads. Why not naming it ExecuteCommands? Because CommandExecutor.Execute(commands) looks better than CommandExecutor.ExecuteCommands(commands) and it shorter. Being short and explicite means the developer is faster in reading, and most probably faster in understanding. And that’s just pure money – beside the fact that the developer may be is even more motivated because he’s not going to be nerved by redundancy.

Actually there are a lot of simple samples. Think of collection methods: Remove, RemoveAll, Clear, Add, Delete. All these are verbs or verb phrases.

Returning string.Empty

What’s wrong with that? The caller will just get a string, right? Yes! That is exactly the point. There are three different reasons why the caller will get the same insensible information.

• The directory is not available
• Getting “ConfigDirectoryInfo” doesnt return a list with items.
• The subdirectories doesn’t contain any items

In fact, returning string.Empty can be useful. But not as part of “Exception Handling”. Because this is no exception handling. This is what I call the “happy case”. This code will do what it has to when everything is available that is necessary. But it will fail ungracefully when there is anything missing. Inspite of taking action apparently, this method let the caller decide what do to when some circumstances, environment information is just not given. That normally lead to

• ackward exception messages
• (even worse) no exception messages, but just swallowing exceptions

It is pretty obvious why exceptions are swallowed. Because the caller doesn’t know anymore what happened it just knows that something went wrong.

What is the solution here? That’s the reason why we do pull requests, to improve before commit. Now the method looks like this:

public string GetVersionNumber(string settingsRootDirectoryName, string settingsDirectorySearchName)
 {
   var rootPath = GetApplicationSettingsRootPath(settingsRootDirectoryName);

   if (!Directory.Exists(rootPath)) throw new DirectoryNotFoundException();

   var softwareAppSettingsDirInfos = GetConfigDirectoryInfo(rootPath, settingsDirectorySearchName);
   if (!softwareAppSettingsDirInfos.Any()) throw new DirectoryNotFoundException();

   var subDirs = new List<DirectoryInfo>();

   foreach (var softwareAppSettingsDirInfo in softwareAppSettingsDirInfos)
   {
      subDirs.AddRange(softwareAppSettingsDirInfo.GetDirectories());
   }

   var versionNumbers = subDirs.Select(subDir => subDir.Name).ToList();

   if (!versionNumbers.Any()) throw new DirectoryNotFoundException();

   var max = 0;
   var maxVersion = string.Empty;

   foreach (var versionNumber in versionNumbers)
   {
      var versionNumberTmp = int.Parse(versionNumber.Replace(".", ""));
      if (versionNumberTmp <= max) continue;
      max = versionNumberTmp;
      maxVersion = versionNumber;
   }
   return maxVersion;
 }

There are still issues. Are you able to identify them? (just scroll down)

• the actual question is: Why is it necessary to iterate through sub directories to get a version number?
• there is a very German typo here: Infos as abbreviation of plural of information which does exist in German but not in English. Actually, the typo should be corrected and there should not be any abbreviations
• the version number is expected to be in a certain format. The dot will be replaced by an empty string, then the code expects to work with numbers. Parse will throw an exception when that doesn’t work. This exception should tell the user something different than “could not parse string and convert it to int” but
  • Where the problem occurred
  • What the problem is
  • And most probably how it can be fixed
• Actually this is not quite visible: The DirectoryNotFoundException had been implemented again even if it is available in System.IO
• GetConfigDirectoryInfo should have a more speaking name. I cannot understand from that name what it actually does and returns.
• “subDirs” is an abbreviation. We aren’t implementing assembler, and we don’t need to save space and memory. So we don’t use abbreviation.

Do you find anything else? Let me know.