Leaderboard

At the company at which I work, we have this legacy application written in C#. (It's different from the one written in Classic ASP.) Let's call this the dragon. It's old, it's previously defeated many brave knights, and its name is an acronym that shares a letter or two with the word "dragon". The dragon is responsible for breathing fire to defend the village from intruders. (It uses coding and algorithms to do Things to Products because of business logic. While the details aren't important, I doubt I could say what it does beyond giving a brief overview of what it looks like to an outside observer.) However, the dragon also has this habit of attacking the knights who control it, so we need a new village gaurdian (the dragon is composed mostly of spaghetti code, and if we are ever to replace it, we need to understand how the spaghetti works). And so, a week or two ago I undertook a challenging task: to shrink the dragon so it'll fear the knights again (to refactor things to make it more readable). Armed with a mouse, keyboard, and the support of a fellow knight (a coworker who's dealt with this application before), I got to work so I could deal a mighty blow to the dragon. (Disclaimer: this post contains a lot of technical C# stuff that can't all be hidden behind dragon metaphores. Hopefully anyone who's not a programmer can still follow.) On my journey of code cleanup, the dragon presented many obstacles. A lot of it was superfluous spacing, redundant comments, and generally painful code, like so: //// //// Configure processing /// this.ConfigureProcessing(); //// //// Do the thing //// try { this.DoTheThing(); } catch { // eat it } if (this.SomeProperty == SomeValue) { // throw new SomeKindOfException(); // this was commented out // to-do: figure out why this was commented out } To make it even better, this.ConfigureProcessing does nothing and is referenced nowhere else, meaning we can easily remove most of the above. Also you should never ever ever ever ever catch an exception without handling it somehow. It will cause pain further down the line (but not in this blog post). If nothing else, log it somehow. Taking all of that into consideration, I reduced that code to this: try { this.DoTheThing(); } catch (Exception ex) { Trace.WriteLine($"Got an exception in TheType.TheFunction: {ex.ToString()}"); } There were also some things outside of general cleanup that I did. Imagine a class like this: public abstract class Magic : IAlgorithm { private string spellName = "Alakazam"; public string SpellName { get { return spellName; } set { if (value == null) { throw new ArgumentException("SpellName can't be null"); } spellName = value; } } private IWizard wizard = null; public IWizard Wizard { get { return wizard; } private set { if (value == null) { throw new ArgumentException("Wizard can't be null"); } wizard = value; } } #region IAlgorithm Methods public void SetWizard(string wizard) { this.Wizard = wizard; } public void Cast() { if (this.Wizard == null) { throw new Exception("Cannot call Execute before SetWizard"); } this.PerformMagic(); } #endregion protected abstract void PerformMagic(); } Not shown: 20 or more classes that inherit from this My fellow adventurer made the observation that the only times SetWizard was called was right after constructors of child classes, kinda like this: public Magic GetMagicInstance(string magicType, string spell, IWizard wizard) { Magic magic = null; switch (magicType) { case "white": magic = new WhiteMagic() { Spell = spell }; magic.SetWizard(wizard); case "black": magic = new BlackMagic() { Spell = spell }; magic.SetWizard(wizard); case "lost": LostMagic magic = new LostMagic() { Spell = spell }; magic.SetWizard(wizard); } return magic; } // Somewhere else calls magic.Cast after all instances of Magic have been created So using this knowledge and the knowledge of new C# features, we made the appropriate changes everywhere to make the class (and related classes) look like this: public abstract class Magic : IAlgorithm { public Magic(string spellName, string wizard) { SpellName = SpellName ?? "Alakazam"; Wizard = wizard ?? throw new ArgumentNullException(nameof(wizard)); } public string SpellName { get; } public IWizard Wizard { get; } #region IAlgorithm Methods public abstract void Cast(); #endregion } They say that if it bleeds, you can kill it. The dragon was certainly bleeding, if the code review tool was anything to go by: there were red lines (removals) all over the place in the pull request (Git terminology basically meaning we're publishing our changes). Two other knights in our party came to inspect our triumph (they reviewed the code to make sure it was done right). When it was all over, a total of 4 people (2 of us were involved in the refactoring) thoroughly reviewed everything and approved. Then we released the changes to be applied to the dragon down in the village (along with some other minor quality-of-life changes). About a week later*, one of the villagers called for help because the dragon that's supposed to defend the village was unable to breathe as much fire (a user opened a support ticket because the Things being done to Products weren't being done correctly). I was skeptical that any of my changes would have that effect. After all, when I shrunk the dragon, I made sure to quintuple-check my changes before releasing it to make sure its fire breath was as unchanged. So I investigated for a few hours to find out why. I ruled out places that could have been affected and was thoroughly stumped. That's when my fellow party member used his influence with the king to learn the secrets known only to the villagers (he used his higher permissions to debug the application, reading data from the production database). That's when I spotted the issue. Remember that constructor in the Magic class that was created? Take another look at the SpellName assignment: SpellName = SpellName ?? "Alakazam"; That should be: SpellName = spellName ?? "Alakazam"; In C#, variables are case-sensitive, so the property was being assigned to itself, and all Magic classes (WhiteMagic, BlackMagic, LostMagic, etc.) all only used the Alakazam spell. No wonder the flames were weak! We fixed the issue, and gave the villagers a dragon capable of properly defending them, and spent the rest of the day identifying how many intruders snuck into the village while the dragon was powerless to stop them (we had to find out which Products didn't have the Thing done properly). Conclusion Case sensitivity can potentially lead to a lot of pain and suffering. In this case with the constructor, that's standard practice in C#, and it's perfectly understandable that no one caught that one mistake, especially since it's just one letter in commit full of thousands of other changes. The tooling didn't catch it, because it checks for useless assignments. This wasn't useless, it says "make SpellName be Alakazam if it's null". Always be careful of casing when variable names are similar. * I massively simplified a lot of things. While a week is probably a bit long to go without noticing anything wrong, there's reasons. Also it didn't affect everything, because it just broke some hacks that are there because business logic.

@borjitasstoi little update for you. I believe these are extracted from SM Demo, that I happened to have lying around on my com. The titles of the .rar should be self-explanatory. 1. SM Dex non-shiny images.rar 2. SM Dex shiny images.rar 3. SM PC healing Sprites.rar What I find fun for the PC Healing images: they've got the alternate sets specifically used for Totem-sized mons XD (I don't have any of USUM ripped images relating to these categories)

Thanks theSLAYER!!

Yes! It worked! Thanks!