Logic can certainly destroy your code reuse, and produce quasi-design smells that you’d really like to do something about because it aggrevates you, but you can’t really change. What if I told you that I had 9 different functions that determine if a date is one calendar year from today’s date? You’d probably gasp, and wonder why in the heck I would need 9 different functions for that. I’ll tell you. In my business domain, 1 calendar year means the following:
Given today is 5/8/2006, 1 calendar year can mean, depending on the context:
5/1/2006 00:00 – 4/30/2007 23:59
5/1/2006 00:00 – 5/8/2007 23:59
5/1/2006 00:00 – 5/31/2007 23:59
5/8/2006 00:00 – 4/30/2007 23:59
5/8/2006 00:00 – 5/8/2007 23:59
5/8/2006 00:00 – 5/31/2007 23:59
6/1/2006 00:00 – 4/30/2007 23:59
6/1/2006 00:00 – 5/8/2007 23:59
6/1/2006 00:00 – 5/31/2007 23:59
Now, which functions gets used depends on the context in which it applies, so puting the functions in a stateful object doesn’t make sense, cause then I’d have actual line for line code resuse because the functions are used throughout different scenarios. Right now, each function exists as a boolean function of a static class. Each function has the following public signature:
public boolean IsWithinValidCalendarYearX(date value)
where X is a some sort of horrible naming convention particular to each function.
Clearly, this is a case for redesign. These functions have been added over time, and I never went back and refactored the logic into something better. Now its time to do so. I could have a private function like:
private boolean IsWithingValidCalendarYear(date startDate, date endDate, date givenDate)
but there is still some work that has to be done in the public function to calculate what the start and end dates are, and an additional parameter to tell it which set of rules to apply, and that possibly makes the API more confusing, unless I used and enum as the 2nd parameter to the public function, which would reduce the confusion. In the end, even though I’ve reduced the number of functions from 10 to 2, the same amount of code still exists, but with less duplicate lines. I could really just have 1 public function, passing in the given date and the enum value and just do all the work right there, because I’m still going to have this mess of conditions and checks regardless of whether I have the private function call or not. At least right now, my cyclomatic complexity is low for each method, but the API itself is ugly.
So where is the trade off? Where does the line lie between ugly API and better code design? Create a nicer API at the expense of more complex code, or leave a complex API at the benefit of less complex methods? What are your thoughts and ideas around this? How would you approach this? The solution to this refactoring will also get applied to similar code smells in the logic that follow the same bad problem.