Refactoring – Fixing huge methods
Cross-posted from my personal blog.
Have you ever had the pleasure to work on a project where acronyms were widely used, or even mandatory, as well as stumbling on methods with more than a hundred (or thousand) lines on a daily basis? I hope you didn’t, because I did, and let me tell you: it’s scary. Allow me to start this post with the following statement:
A great developer is one who can make code which a beginner can understand
That’s not very poetic I know, but I think you can see what I’m trying to transmit here. One shouldn’t be proud of writing overly complex code. Sure, you can always find a way to write a method in one line, but the question is: should you?
In case you are new to the world of development, first of all, welcome, it’s a wild ride. Let us start with a bad piece of code and walk through it until we can polish it into something readable. Previously, we were developing our brand new attack robot. Seems like we did a good job, and now management asked us to work on its shield module. Let’s get into it!
class ShieldSystem
{
private readonly RoboSystem? _system;
private readonly EolicPowerSource _eolicPowerSource;
private readonly SolarPowerSource _solarPowerSource;
private readonly OutOfNowherePowerSource _outsideNowherePowerSource;
public void Shield()
{
if (_system != null && _system.IsOnline)
{
try
{
IShield? shield;
if (_system.OverallBatteryLevel > 80)
shield = new StandardShield();
else
{
if (_system.HasEmergencySupport)
shield = new EmergencyShield();
else
throw new Exception("ERR00");
}
if (shield != null)
{
if (shield is StandardShield)
{
var shieldBattery = new ShieldBattery();
if (_eolicPowerSource != null)
shieldBattery.DoWork(_eolicPowerSource);
else if (_solarPowerSource != null)
shieldBattery.DoWork(_solarPowerSource);
else if (_outsideNowherePowerSource != null)
shieldBattery.DoWork(_outsideNowherePowerSource);
if (shieldBattery.Percentage != 100)
{
throw new Exception("ERR01");
}
}
shield.Activate();
}
}
catch (Exception ex)
{
if (ex.Message == "ERR01")
{
var mailServerAddress = "www.google.com";
if (new Ping().Send(mailServerAddress).Status != IPStatus.Success)
mailServerAddress = "www.googlebutpingable.com";
var mailClient = new MailClient(mailServerAddress);
mailClient.SendMail(@"Hello" +
@"It seems one of our power sources didn't really charge the battery." +
@"Thats a bummer." +
@"Please investigate ASAP!");
}
}
}
}
}
Tada! Simple as that, we have a great shield system. It even has a try/catch block, so it’s definitely fail-proof. This will surely grant us our so desired promotion, right? Well, it won’t, let’s see what are the problems of this piece of code:
In order to fix the issues in our method, the easiest way to start is to figure out how to make it more readable. We definitely need to split this code into smaller chunks, so let’s proceed by extracting some methods. Looking at our code, we can see that it does a couple of things:
- Verify the system status (if it’s online)
- Get an IShield instance
- Charge the shield
- Activate it
- Notify someone if it fails to charge
To begin with, let’s create a separate method for each step of this huge method. Being able to read a method like steps helps a lot regarding its readability. Besides that, we want to fix the Single Responsibility ASAP. So, let’s see how it turns out after some refactoring magic:
class ShieldSystem
{
private readonly RoboSystem? _system;
private readonly EolicPowerSource _eolicPowerSource;
private readonly SolarPowerSource _solarPowerSource;
private readonly OutOfNowherePowerSource _outsideNowherePowerSource;
public void ActivateShield()
{
if (!SystemIsOnline())
return;
try
{
var shield = GetAvailableShield();
if (shield == null)
throw new Exception("ERR00");
if (shield is StandardShield standardShield)
ChargeShield(standardShield);
shield.Activate();
}
catch (Exception ex)
{
NotifyPowerChargingIssue(ex);
}
}
private void NotifyPowerChargingIssue(Exception ex)
{
if (ex.Message != "ERR01")
return;
var mailServerAddress = GetMailServerAddress();
var mailClient = new MailClient(mailServerAddress);
mailClient.SendMail(@"Hello" +
@"It seems one of our power sources didn't really charge the battery." +
@"Thats a bummer." +
@"Please investigate ASAP!");
}
private string GetMailServerAddress()
{
var mailServerAddress = "www.google.com";
if (new Ping().Send(mailServerAddress).Status != IPStatus.Success)
mailServerAddress = "www.googlebutpingable.com";
return mailServerAddress;
}
private void ChargeShield(StandardShield standardShield)
{
var shieldBattery = new ShieldBattery();
var powerSource = GetPowerSource();
if (powerSource == null)
throw new Exception("That's a new error!!");
if (shieldBattery.Percentage != 100)
throw new Exception("ERR01");
standardShield.Charge(shieldBattery);
}
private IPowerSource? GetPowerSource()
{
if (_eolicPowerSource != null)
return _eolicPowerSource;
if (_solarPowerSource != null)
return _solarPowerSource;
if (_outsideNowherePowerSource != null)
return _outsideNowherePowerSource;
return null;
}
private IShield? GetAvailableShield()
{
if (_system.OverallBatteryLevel > 80)
return new StandardShield();
if (_system.HasEmergencySupport)
return new EmergencyShield();
return null;
}
public bool SystemIsOnline()
=> _system != null && _system.IsOnline;
}
Much better right? Now just with a quick glance, we can already figure out what the “Shield” method does, which we renamed to “ActivateShield” since it clearly violates our method naming best practices. Still, we have code smells to solve. A class that activates shield should not be sending mail right? Also, I’m pretty sure we can extract the battery charging behavior into a separate class. Let’s see.
class ShieldSystem
{
private readonly RoboSystem? _system;
private readonly IShieldCharger _shieldCharger;
private readonly RoboMailer _mailer;
public void Shield()
{
if (!_system.IsOnline)
return;
try
{
var shield = GetAvailableShield();
if (shield == null)
throw new Exception("ERR00");
if (shield is StandardShield standardShield)
_shieldCharger.ChargeShield(standardShield);
shield.Activate();
}
catch (Exception ex)
{
if (ex.Message == "ERR01")
_mailer.NotifyPowerChargingIssue();
}
}
private IShield? GetAvailableShield()
{
if (_system.OverallBatteryLevel > 80)
return new StandardShield();
if (_system.HasEmergencySupport)
return new EmergencyShield();
return null;
}
}
Nice right? We also changed our “if (!SystemIsOnline())” line to directly verify the System’s IsOnline property because let’s agree that if the system is null, something is very wrong with this software. Now, we have to solve our try/catch block because it’s catching ANY exception. You should always be careful when catching an Exception instead of specific Exceptions (like an ArgumentException) because you can accidentally mask errors and get a much worse bug. For more details, be sure to check our exception handling best practices.
Anyway, there are two ways that we can solve this, the first one is like this:
catch (Exception ex) when (ex.Message == "ERR01")
{
_mailer.NotifyPowerChargingIssue();
}
That’s the simplest way, we are just handling exceptions with that specific message. It fixes our problem, but it’s not the cleanest solution. The best solution is to create a new specific Exception to that case and then throwing it. You might be asking yourself “Why the hell are you teaching me this then?”, right? Well, sometimes you just can’t change the thrown exception, because that code might be from a third-party provider, for example, so it’s always good to know. Anyway, our catch block should look like this:
catch (PowerChargingException)
{
_mailer.NotifyPowerChargingIssue();
}
Another point of improvement is to remove the magic strings “ERR00” and “ERR01”. They aren’t necessarily bad, but in the long run they can cause a lot of trouble. You can read more about it here. There are plenty of ways to solving them, but we will keep it simple. And so, our final code:
class ShieldSystem
{
public const string ShieldErrorCode = "ERR00";
private readonly RoboSystem? _system;
private readonly IShieldCharger _shieldCharger;
private readonly RoboMailer _mailer;
public void Shield()
{
if (!SystemIsOnline())
return;
try
{
var shield = GetAvailableShield();
if (shield == null)
throw new Exception(ShieldErrorCode);
if (shield is StandardShield standardShield)
_shieldCharger.ChargeShield(standardShield);
shield.Activate();
}
catch (PowerChargingException)
{
_mailer.NotifyPowerChargingIssue();
}
}
private IShield? GetAvailableShield()
{
if (_system.OverallBatteryLevel > 80)
return new StandardShield();
if (_system.HasEmergencySupport)
return new EmergencyShield();
return null;
}
public bool SystemIsOnline()
=> _system != null && _system.IsOnline;
}
Refactoring takes a lot of work, but it is necessary in order to keep our code base healthy. Methods which are too long will always be great candidates for refactoring. When you decide to get your hands on refactoring, try to identify behaviors inside it so you can extract smaller methods. Also, remember to always keep the SOLID guidelines in mind.
