Title: Student Code Critique Competition

So how on earth do we explain or demonstrate how to think clearly? Perhaps it is worth taking a step further back from the impossible and showing how a clear problem definition can make it easier to think clearly about the problem in hand. It doesn't remove the need for clear thought, but it provides a sieve that you can use to filter out the rubbish that might be cluttering your thoughts. The better the problem definition the finer the sieve and, one hopes, the clearer the resulting thoughts.

Let me try and demonstrate the process.

Taking the original summary of the programme's purpose it looks like the programme should report the user's age in three different formats rather than as a single, "You are 27 years, 6 months and 21 days old" result. Why? Well, normally you would not report month values greater than 11 or days greater than 364. If that is the case then the sample data doesn't make this clear as the figures yield 3 different ages, 27, not quite 17 and a little over 4 years. However looking at the code, well I'm really not sure what it's trying to do, but I think it may in fact be trying to report the age as a single, "You are x years, y months and z days old." statement.

So how are we to solve this problem? Which problem? Good point, let's step back a bit and see if we can identify what problems we have. In no particular order:

Well the first problem on the list is probably the least important of them all, it will come down to (ha ha) a SMOP (simple matter of programming) and providing the rules used to calculate ages are followed correctly any one solution is as likely to be as good as any other.

The last question is always worth asking. Is this a one off task where the result can be obtained using a pencil and paper with, perhaps, the backing of an electronic calculator?

Correct answers to the remaining questions are vital to the successful outcome of our project. So how are we going to answer these questions? Simple. We're going to have to ask someone and more to the point someone who knows about the problem area.

The really difficult part of our trade is that there's only one way of finding answers to the questions above. That is talking to other people and extracting information and decisions from them. This is a skill every bit as important as being able to think clearly or knowing how to write the leanest, meanest most efficient code ever in the most fashionable language of the moment.

So how might this go in the commercial world? Well let's imagine....

... and so it went on. Our heroine ploughed through a list of questions that she had, adding others that arose out of the discussion. Then, after a good deal of explanation that a programme written to a limited specification would be quicker to produce and above all would be far, far, far cheaper to produce than one that dealt gracefully with locale issues, she presented the following to Colonel Blimp her customer...

All that work, for such a simple programme and we haven't even begun to suggest any solutions never mind cutting any code.

By the way there's a bear trap in the foregoing. The customer may not be, is rarely, the user and may just have led you right up the garden path. So in the ideal case we'd go and talk to some typical users as well before we went any further. That's the ideal though...

Well what we've got is not complete, first attempts almost never are, but it is a clearer statement of the problem that we can discuss with colleagues and it can be used as a basis for sensible design decisions and, just as important, we have the beginnings of something that we can test against.

So why would we want to discuss our problem definition further? Well, let's start with date entry. We want to prevent the entry of dates in advance of the current date. This might be simpler if the user entered the complete date at once, but then we have the complicating factor of format validation. Then there's the matter of valid year ranges. We've said "130 years before current date" and given an example. But do we really mean 'current date' or should we have said 'current year'? And what about that 130 limit itself? What happens if we want to know the age of an elderly Galapagos tortoise? Oh, and how do we define a 'complete' month and what about someone whose birthday falls on the 29th of Feb?

So, where are we in our attempt to show at least one way of reducing vulnerability to muddled thought?

At this point you can then sit back under your (metaphorical) tree and try and feed candidate solutions through your sieve.

I haven't got much to say about this as, as I said earlier, how on earth do you teach clear thinking? So perhaps a list of points that work for me will at least give you something to start with…

So what about the original code? Well as the original statement of requirements is a little vague there's not a lot that I can say and I have as many questions as criticisms.

Well having written all that the very least I can do is offer a possible solution to what might have been the original problem. It is flawed, but then you, dear reader, are the first reviewer of the work, the work was done without collaboration and (through no other fault but my own) I left it until too close to the deadline to start so poor design choices have been left as they were. Now, remind me, what was I saying earlier? :)

// Warning! Only (lightly) tested with g++ on
// FreeBSD 4.6.2 For student code critique 23.

#include <iostream>
#include <ctime> // in g++ this is an alias

// for time.h - it may not always be so.
using namespace std;
// Assumptions -
// Gregorian calendar is going to be used.
// We don't expect to find anyone older than
// about 125.
// All sorts of assumptions about current
// date, time and we had
// to have a look at time.h to check what we'd get back.
const int MAX_LIFETIME_YEARS = 130;
const int SYS_BASE_YEAR = 1900;
const int MONTHS_IN_YEAR = 12;
enum eMonth {
  Unknown,
  January = 1, February, March, April, May,
  June, July, August, September, October,
  November, December
};

bool IsLeapYear(int Year) {
// Leap years in the Gregorian calendar occur
// in years that are exactly divisible by 400.
// Non-centenary years that are exactly
// divisible by 4.
  if(Year % 100 == 0 && Year % 400 == 0)
    return true;
  if(Year % 100 != 0 && Year % 4 == 0)
    return true;
  return false;
}
int DaysInMonth(int Year, eMonth Month) {
/* Thirty days hath September, April, June &
   November, All the rest have thirty-one.
   Excepting February which hath 28 except...
*/
  int days = 31;
  switch (Month) {
    case February:
      if(IsLeapYear(Year))
        days = 29;
      else
        days = 28;
      break;
    case September:
    case June:
    case April:
    case November:
      days = 30;
      break;
  }
  return days;
}

int main() {
  int Day = 0;
  int Month = 0;
  int Year = 0;
 
  // We mustn't allow anyone to enter a date
  // later than the current date.
  time_t NowInSeconds = 0 ;
  tm* pCurrentDate;
  int CurrentYear = 0;
  int CurrentMonth = 0;
  int CurrentDay = 0;

  // Find out what the current date is.
  time(&NowInSeconds);
  pCurrentDate = localtime(
    const_cast<time_t* > (&NowInSeconds));
  CurrentYear = SYS_BASE_YEAR +
                pCurrentDate->tm_year;
  int ApplicationBaseYear =
                CurrentYear - MAX_LIFETIME_YEARS;
  CurrentMonth = 1 + pCurrentDate->tm_mon;
                CurrentDay = pCurrentDate->tm_mday;

  // Let's keep a track of where we are.
  cout << "Currently : " << CurrentYear << "/"
       << ((CurrentMonth < 10) ? "0" : "")
       << CurrentMonth << "/"
       << ((CurrentDay < 10) ? "0" : "")
       << CurrentDay << '\n';

  while(Year < ApplicationBaseYear
        || Year > CurrentYear) {
    cout << "Please enter a year of birth: ";
    cin >> Year;
    if(Year < ApplicationBaseYear
       || Year > CurrentYear)
      cout << "\tPlease enter a year from "
           << ApplicationBaseYear
           << " to " << CurrentYear << '\n';
  }

  int MaxMonth = MONTHS_IN_YEAR;
  if(Year == CurrentYear)
    MaxMonth = CurrentMonth;
  while(Month < 1 || Month > MaxMonth) {
    cout << "Please enter the month of birth: ";
    cin >> Month;
    if(Month < 1 || Month > MaxMonth) {
      cout << "\tA month from 1 to "
           << MaxMonth << " please.\n";
      Month = 0;
    }
  }

  int MaxDay = DaysInMonth(Year,
                 static_cast<eMonth>(Month));
  if(Year == CurrentYear
     && Month == CurrentMonth)
    MaxDay = CurrentDay;
  while(Day < 1 || Day > MaxDay) {
    cout << "Please enter a day of birth: ";
    cin >> Day;
    if(Day < 1 || Day > MaxDay) {
      cout << "\tA day between 1 and "
           << MaxDay << " please.\n";
      Day = 0;
    }
  }

  int WholeYears = CurrentYear
                   - Year - (CurrentYear>Year? 1 : 0);
  int WholeMonths = 0;
  int WholeDays = 0;

  if(CurrentMonth == Month) {
    if(CurrentDay >= Day) ++WholeYears;
    else WholeMonths = MONTHS_IN_YEAR-1;
  }
  else if(CurrentMonth > Month) {
    if(CurrentYear > Year) ++WholeYears;
    WholeMonths = CurrentMonth - Month- 1;
  }
  else {
    WholeMonths =
      MONTHS_IN_YEAR - Month + CurrentMonth;
    if(CurrentDay < Day) -WholeMonths;
  }
  if(CurrentDay >=Day) {
    WholeDays = CurrentDay -Day;
  }
  else {
    int PreviousMonth = (CurrentMonth + 
          MONTHS_IN_YEAR -1) % MONTHS_IN_YEAR;
    WholeDays = DaysInMonth(CurrentYear,
    static_cast<eMonth>(PreviousMonth)) - Day;
    if(WholeDays < 1) WholeDays = 0;
      WholeDays = WholeDays + CurrentDay -1;
  }

  cout << "Age at end of "
       << CurrentYear << "/"
       << ((CurrentMonth < 10) ? "0" : "")
       << CurrentMonth << "/"
       << ((CurrentDay < 10) ? "0" : "")
       << CurrentDay << " was "
       << WholeYears << " year"
       << ((WholeYears != 1) ? "s " :" ")
       << WholeMonths << " month"
       << ((WholeMonths != 1) ? "s" :"")
       << " and "
       << WholeDays << " day"
       << ((WholeDays != 1) ? "s." :".\n");
  return 0;
}

Colin's code caused me some problems because there is a relatively large amount of it and in the original he had added a considerable amount of comment. Space is always at a premium in C Vu so I removed most of the comments. The result may be less readable but it is almost a page shorter, which, in this context, matters.

There are several points that I want to comment on before moving on to the next entry.

To my mind main is clearly far too large and includes far too many nested structures. While Colin has carefully validated the input in regards to the numerical values he has not checked for successful input. In real life, in my experience, the latter problem is far more common.

A large proportion of his code (and most of the nested conditionals) is the result of the way he has approached the problem. How could he have done it differently? Consider:

int years = current_year birth_year;
int months = current_month - birth_month;
int days = current_day - birth_day;
if(days < 0) {
  --months;
  days += days_in[leap][current_month-1]
}
if(months < 0) {
  --years;
  months += months_in_year;
}
if(years < 0) {
  cout << "You haven't been born yet.\n";
else
  cout << "Your age is:"
// etc

Oh, that days is in because I would always use a look up table for this kind of irregular data. In this case I would store the number of days in December as both the first and the last entry. Check that out, it ensures that January works correctly even when the current date is earlier than a January birthday. The other advantage of a look-up table is that it makes the validation code for birth_day much simpler. leap will have been evaluated earlier.

One thing always worth considering is distinguishing between unlikely data and impossible data. The claim to have been born in 1503 is (extremely) unlikely but the claim to have been born in the 15th month or the 33rd day of the seventh month is impossible (for a Gregorian Calendar). I tend to favour accepting unlikely data but perhaps verifying it.

(Editorial note: I would not use a lookup table here as - to my mind - it adds complexity rather than clarity. But that just shows that simplicity and elegance is hard to pin down, and even experienced programmers can reasonably disagree sometimes. James)

Firstly

void main()

might often be written as

int main(void)

together with a

return 0;

at the end of function main.

[Not strong enough. It should be written with an int return type unless you are happy to write non-portable code relying on a vendor extension. Francis]

Then you declare

int currentyear = 2003;

This works this year, but you would have to rewrite the program next year. Ideally one would get the program to use the system to tell you the current date. However you seem to have forgotten what you've done when you write

result = dob - 2003;

using the 'magic number' 2003 rather than

result = dob - currentyear;

However suppose dob is 1983, then result = 1983 - 2003 = -20. You should have said

result = currentyear - dob;

This raises the question of naming variables. I suppose dob is 'date of birth', but it is used in the program in the sense of 'year of birth'. Does the difference matter? Yes. There are a variety of naming conventions, but it is generally accepted that variable names should reflect as closely as possible the 'meaning' of the variable, and this often prompts you to ask yourself 'what exactly is this variable about?' which is often a very useful question. You can probably now see the problem with the name 'result'.

We then have

for(result=0; result <=12; result++)
if(result <=12) result = month + 12;
  cout << "You are " << result
       << "months old" << endl;

Let's see what this will do. The value of month will be between 1 and 12 (if the user entered a valid value). The first time into the loop, result will be 0, so

result = month + 12;

will give result a value between 13 and 24. Then result++ will add 1 to this, and since result will be 14 or more, the loop will terminate. If I was born in January it will tell me I'm 14 months old.

So what did you think it would do? You have a ' + 12' inside a loop - so are you adding 12 for each year of the person's life? If so you would have needed to say

result += 12;

then added month in once after the loop for the incomplete year. But

Using more precise variable names, such as yearOfLife or monthOfLife, might have made the problem slightly more apparent to you.

You repeat the same code pattern in

for(result=0; result<=365; result++)
  if(result<=365) result = day + 365;
    cout << "You are " << result << " days old"
         << endl;

Before you repeat a code pattern - make sure the original works.

We start from the question -

You say it should 'tell the user how old they are in years, days and months i.e. 27 years 200 months and 1500 days old'

I think you mean "e.g." not "i.e." Now this might mean how old in complete years, complete months and days. Alternatively it might mean how old in years (ignoring part years) or in months (ignoring part months) or in days (ignoring... you've got it). Your example seems to suggest the latter, but not clearly, since someone who was exactly 27 years old would have be 324 months old, not 200. Before writing the program, you must be clear as to what it is supposed to do. Let's take the second interpretation here.

Einstein is supposed to have said

which has been made into the acronymn KISS, keep it simple, stupid. However Einstein added the more useful caution

In the context of calendars, dates and times, 'not too simple' is really quite complicated. For a start, what do you mean by month - calendar or lunar? Well calendar is a lot easier here, so we'll do that. As another example, calculating the number of complete years involves not just the year of birth and the current year. For example, the current date is 21 August 2003. If my date of birth was say 1 November 2002, my age in complete years is not 2003 - 2002 = 1. It is zero - I have not yet lived 12 months. Similar ideas apply for months and days. In this way we have a more precise specification for the program:

How do we do it? Well, you need to re-submit this assignment so I will not do it for you, just outline two approaches. The first is to use calendar and date library functions which will essentially do it for you. However that is not what you are expected to do here. The second approach is to do it yourself, as follows:

Firstly calculate the complete years. You would need to compare the current month and the month of birth, and if they are equal, the birth day compared to the current day. It might help to draw some diagrams, or work out some examples on paper.

Then calculate the complete months. This is the complete years times 12 plus the difference in the months.

Then the days. The simple approach is the complete years multiplied by 365, plus the days in the months - which depends on which months they are.

The correct approach takes leap years and leap centuries into account - and this is what makes libraries so nice.

Interesting that both submissions religiously use endl. What is wrong with '\n' which does exactly what you want unless you really do need to flush the output. I actually edited most of Colin's uses of endl but left Walter's in so I could make this comment.