Making Code Maintainable

One of the things I’ve learned over the years is to recognize when some code should be changed to be more maintainable. I’ll walk you through some code I saw during a pull request that we ended up refactoring to be more maintainable.

We were calling an external API that returns string “yes” and “no” for a bool value sometimes, and other times it sends a “true” and “false”. Obviously we wanted to accept both and convert them appropriately. The first version to accept these values was to have a property that checked the values of itself to let us know if it was true or false, it looked like this:

When we were mapping to our model, we were simply doing this:

When reviewing, we recognized two main issues. The first issue is that our values are duplicated, so if the API started sending us 1 and 0, then we might go update one area and forget to correct the other section. We could write some tests to let us know, but we didn't want to risk it, and of course there's a better way. There's always a better way. The other issue was that if one of the other fields that was sent to us was a "yes", but not actually a bool, then we'd be in trouble.

So, how did we solve the two issues? This is how:

With the fix, you'll notice that we're checking our destination model type now instead of the field's IsABooleanProperty. This fixes our second issue of getting an invalid bool. Our first problem is fixed by having the valid bool values a single time in a convert to bool method, and getting rid of the IsABooleanProperty. If you have other approaches to how to solve this problem or more examples of making code maintainable, please comment!

Good Ole Refactoring

I was talking with some co-workers about a pull request recently and we all decided we didn’t really like the look of some code.

Here's the code:

Our main issue with the code was that it was difficult to read and immediately understand what was going on.

Here are the things you should know:

• The SystemClock is a Headspring nuget package
• IsDayOnWeekend just checks if the current day is a Saturday or Sunday
• IsHoliday checks whether or not the day falls on a holiday

So we did a small refactoring to make the code more readable.

• So we renamed the method to include GetPreviousBusinessDay
• We get the last 365 days, we could've just as easily gotten the last 30, but decided to just get the full year.
• After we get the last 365 days, we get only the valid business days
• Then we order those dates descendingly, skip the number of days back we want to go, and then grab the first date

Our refactoring is more readable, has some "clever" code that we like, but aren't married to either. I'm curious to know other opinions, and other solutions that we could've used.