How I introduced code with one small mistake that confused other devs

Sometimes when we write code we think it is good enough and should be understandable by the others, but after a while we realize how wrong we were and how much truth is in the saying “Expect the unexpected“. It was similar in my case.

One of the features that I was working on was extending the method to create new configuration. The method sent an event with created configuration and I was supposed to extend it to pass another parameter. The parameter was DeviceType and could be taken from the connected device, user preferences or selected directly in the modal window. The implementation I found good enough looked similar to this one

public void CreateConfiguration()
{
   var deviceType = DeviceType.None;
   
   if (deviceController.IsConnected) 
   {
      deviceType = mapper.Map<DeviceType>(deviceController.Device);
   }
   else if (userPreferences.HasValue(Constants.DEVICE_TYPE_PEREFERENCE_KEY)) 
   {
      deviceType = userPreferences.GetValue(Constants.DEVICE_TYPE_PEREFERENCE_KEY);
   }
   else 
   {
      var deviceTypeSelectionModal = modalResolver.Resolve<IDeviceTypeSelectionModal>();
      if (deviceTypeSelectionModal.Show() == ModalResults.OK) 
      {
         deviceType = deviceTypeSelectionModal.DeviceType;
      }
   }

   if (deviceType != DeviceType.None) 
   {
      eventsFactory.Push(new ConfigurationCreatedEvent(deviceType));
   } 
}

I decided that the method was simple and breaking it down wouldn’t make it much better. After some time, other developer had to pass another parameter, but only for a specific case – connected device of a certain type. His code was as follows (I skipped lines that have not changed to improve readability)

public void CreateConfiguration()
{
   ...
   string deviceConnectionPort;

   if (deviceController.IsConnected) 
   {
      deviceType = mapper.Map<DeviceType>(deviceController.Device);
      
      if (deviceType == DeviceType.DEVICE_A) 
      {
         deviceConnectionPort = deviceController.Port;
      }
   }
   ... 
}

The method is growing and is already a good candidate for refactoring, but the code still works as intended. During code analysis by the team, we noticed that enums Device from DeviceController and DeviceType are identical, and both are used internally by us, they are not part of any contract or anything like that, so it would be a good idea get rid of one of them.

We decided to replace Device with DeviceType, but during refactoring, we missed the fact that the developer who made the fix thought that the assignment to deviceType is only to use it later in the condition, so since we don’t need to map types anymore, we can skip assignment. Refactored code looked something like this

public void CreateConfiguration()
{
   ...
   if (deviceController.IsConnected &&
       deviceController.DeviceType == DeviceType.DEVICE_A) 
   {
      deviceConnectionPort = deviceController.Port;
   }
   ...
}

There are a few more inaccuracies that may have come to your mind and for which I owe you an explanation. The first thing has already been explained, the pull request contained a lot of simple changes, so it drop our guard. In that case, you may ask where were unit tests and CIs? Unfortunately, at that time, CI was in the middle of transferring to another server and we decided not to block development for this reason. Unit tests for this functionality existed, but the developer chose not to run them for reasons known to him.

The end of the story is as follows, the QA team detected and reported bug very quickly. The whole thing could have been detected in the earlier phases, but by a coincidence, this story taught me how misleading intention that I introduced evolved over time and became serious issue.

Lesson learned

What have I learned from this story? The code may seem simple, but you still have to remember to make sure that the intention is clear and it doesn’t allow someone to lose it by mistake. Due to the fact that I left the code for receiving DeviceType in the method to create the configuration, I allowed to get other values in the same place and therefore lost the original intention. This is how the code should look like, so that his intention is clear

public void CreateConfiguration()
{
   var deviceType = GetDeviceType();  
   if (deviceType != DeviceType.None) 
   {
      eventsFactory.Push(new ConfigurationCreatedEvent(deviceType));
   } 
}

private DeviceType GetDeviceType()
{   
   if (deviceController.IsConnected) 
   {
      return mapper.Map<DeviceType>(deviceController.Device);
   }
   if (userPreferences.HasValue(Constants.DEVICE_TYPE_PEREFERENCE_KEY)) 
   {
      return userPreferences.GetValue(Constants.DEVICE_TYPE_PEREFERENCE_KEY);
   }

   var deviceTypeSelectionModal = modalResolver.Resolve<IDeviceTypeSelectionModal>();
   if (deviceTypeSelectionModal.Show() == ModalResults.OK) 
   {
      return deviceTypeSelectionModal.DeviceType;
   }

   return DeviceType.None;
}

There is still another lesson that is most simply described by the quote

Anyhting that can go wrong will go wrong

Murphy Law

The number of complications is growing very quickly and due to the fact that our work is hardly repetitive, it’s difficult to develop many habits that can effectively eliminate this type of problems, but my knowledge in this subject also increases, so I’m also getting closer to the point in which I will be able to effectively counteract such cases.

This is the first post about stories that happened to me, so please let me know if you find them valuable and whether I should write more about my experience. If you have stories that have taught you valueable lessons, I encourage you to share!

1 thought on “How I introduced code with one small mistake that confused other devs”

  1. Pingback: dotnetomaniak.pl

Leave a Comment

Share via
Copy link
Powered by Social Snap