CodeSOD: Saved Changes

When you look at bad code, there's a part of your body that reacts to it. You can just feel it, in your spleen. This is code you don't want to maintain. This is code you don't want to see in your code base.

Sometimes, you get that reaction to code, and then you think about the code, and say: "Well, it's not that bad," but your spleen still throbs, because you know if you had to maintain this code, it'd be constant, low-level pain. Maybe you ignore your spleen, because hey, a quick glance, it doesn't seem that bad.

But your spleen knows. A line that seems bad, but mostly harmless, can suddenly unfurl into something far, far nastier.

This example, from Rikki, demonstrates:

private async void AttemptContextChange(bool saveChanges = true) { if (m_Context != null) { if (saveChanges && !SaveChanges()) { // error was already displayed to the user, just stop } else { dataGrid.ItemSource = null; m_Context.Dispose(); } }}

if (saveChanges && !SaveChanges()) is one of those lines that crawls into your spleen and just sits there. My brain tried to say, "oh, this is fine, SaveChanges() probably is just a validation method, and that's why the UI is already up to date, it's just a bad name, it should be CanSaveChanges()" . But if that's true, where does it perform the actual save? Nowhere here. My brain didn't want to see it, but my spleen knew.

If you ignore your spleen and spend a second thinking, it more or less makes sense: saveChanges (the parameter) is a piece of information about this operation- the user would like to save their changes. SaveChanges() the method probably attempts to save the changes, and returns a boolean value if it succeeded.

But wait, returning boolean values isn't how we communicate errors in a language like C#. We can throw exceptions! SaveChanges() should throw an exception if it can't proceed.

Which, speaking of exceptions, we need to think a little bit about the comment: // error was already displayed to the user, just stop.

This comment contains a lot of information about the structure of this program. SaveChanges() attempts to do the save, it catches any exceptions, and then does the UI updates, completely within its own flow. That simple method call conceals a huge amount of spaghetti code.

Sometimes, code doesn't look terrible to your brain, but when you feel its badness in your spleen, listen to it. Spleen-oriented Programming is where you make sure none of the code you have to touch makes your spleen hurt.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!