Cleaning Up More Code – I Need a Bigger Bucket
Today I'm slugging through the clean-up of the last bit of work done by others on this project of mine, and while it's not as bad as some of the other code I've had to clean up, there's still a considerable bunch of problems in the way a lot of things were done. For example, our internal representation of a date is a single unsigned integer value of the form YYYYMMDD. It's simple, easy to compare, and very easy to read by a human. However, for the calculation model we need to use, they have a simple structure with three parts: year, month, day. Each is a simple integer, and while I can see why they might want to do that, they don't have any of the supporting code to make it easy to do comparisons, etc. None of this really matters because that's just what we have to do. So be it.
But the problem I saw in the code was that there was a method to convert from our representation to theirs, and it was written as:
void Calculator::getDate( uint32_t aDate, uint32_t & aYear, uint32_t & aMonth, uint32_t & aDay) { aYear = aDate / 10000; aMonth = (aDate - aYear * 10000)/100; aDay = (aDate - aYear * 10000 - aMonth * 100); }
and in the code using this method was called:
getDate(expir, model.year, model.month, model.day);
and there was at least one real problem with this, and one really bad design choice. First, let's look at the problem. The date structure is defined as:
struct FEDate { int day; int month; int year; };
and from clever inspection, we're mapping these signed integers to the unsigned integers of the method. Well... the code really looks like:
getDate(expir, (uint32_t &)model.year, (uint32_t &)model.month, (uint32_t &)model.day);
when it's not a really great idea to say the references are to unsigned values when they are going to be used as signed ones. The horrible design issue is that there's never a case we need to have this as the separate components - we're much better off dealing with the conversion to the struct directly:
void Calculator::fillDate( uint32_t aDate, FEDate & aTarget ) { aTarget.year = aDate / 10000; aTarget.month = (aDate - aTarget.year * 10000)/100; aTarget.day = (aDate - aTarget.year * 10000 - aTarget.month * 100); }
Now we don't have to worry about the conversion issue at all. Plus, it's clear that we're converting from our representation to theirs. Simple. Clean. Direct. Much better.
I'm going to have to go through a lot of the code like this, finding little things that are still causing memory leaks - and there are at least a few here. I need to get them all cleaned out as quickly as possible as we have the Developer Day tomorrow, and I'd really like to have this all done today.