3
\$\begingroup\$

I'm giving a lecture on basic programming to first-year mathematics students. The programming language is Java (defined by curriculum).

Last week I gave the following task:

Write a program, that reads in the date of birth of the user separated into year, month and day and subsequently calculates the days the user has lived until today.
Do consider leap years.

This task seems not to be too hard, but there are some restrictions (as they are first-year students and never had had any experiences in programming):

  1. only the basic data types are known (int, double, boolean, String etc.)
  2. arrays are not known
  3. writing own functions is not known (i.e. everything is within the main)
  4. except of System.out.print/ln none of the Java API functions are known
  5. user input is realised through a provided method readInteger(String promt) (the students just use it, but don't know why and how it works)

Here I present my sample solution and would like to know, whether there are some more possibilities of "smoothing" the code, optimising it or just increasing the readability. Only tweaks and tricks are allowed, which can be done given the knowledge-restrictions above.

public static void main(String[] args) {
    // define today's day
    final int TODAY_YEAR = 2011, TODAY_MONTH = 4, TODAY_DAY = 29;
    // define a leapyear in the past
    final int LEAPYEAR = 1904;
    // is the current year a leapyear?
    boolean todayIsLeapyear = false;

    // get user input
    int birthYear = readInteger("Please enter the year of your birth:");
    int birthMonth = readInteger("Please enter the month of your birth:");
    int birthDay = readInteger("Please enter the day of your birth:");

    // check input for validity
    boolean inputOK = true;
    int birthYearLeapShift = 0;
    boolean birthYearWasLeap = false;
    if (birthYear > 2011 || birthMonth > 12) {
        System.out.println("Your input was not correct.");
        if (birthYear > 2011) {
            System.out.println("You are from the future! How is it there?");
        } else {
            System.out.println("We will calculate in the usual gregorian calendar.");
        }
        inputOK = false;
    } else {
        // calculate shift of year of birth to the prior leapyear
        birthYearLeapShift = (birthYear - LEAPYEAR) % 4;
        birthYearWasLeap = (birthYearLeapShift == 0);

        // check the input of the month of birth for validity
        switch (birthMonth) {
            case 1: //january
                inputOK = (birthDay <= 31);
                break;
            case 2: //february
                inputOK = (birthDay <= 28);
                inputOK = (birthDay <= 29 && birthYearWasLeap);
                break;
            case 3: //march
                inputOK = (birthDay <= 31);
                break;
            case 4: //april
                inputOK = (birthDay <= 30);
                break;
            case 5: //may
                inputOK = (birthDay <= 31);
                break;
            case 6: //june
                inputOK = (birthDay <= 30);
                break;
            case 7: //july
                inputOK = (birthDay <= 31);
                break;
            case 8: //august
                inputOK = (birthDay <= 31);
                break;
            case 9: //september
                inputOK = (birthDay <= 30);
                break;
            case 10: //october
                inputOK = (birthDay <= 31);
                break;
            case 11: //november
                inputOK = (birthDay <= 30);
                break;
            case 12: //december
                inputOK = (birthDay <= 31);
                break;
        }
    }

    if (inputOK) {
        // calculate the days fo the current year
        int daysInCurrYear = TODAY_DAY;
        switch (TODAY_MONTH - 1) {
            case 11:
                daysInCurrYear += 30;
            case 10:
                daysInCurrYear += 31;
            case 9:
                daysInCurrYear += 30;
            case 8:
                daysInCurrYear += 31;
            case 7:
                daysInCurrYear += 31;
            case 6:
                daysInCurrYear += 30;
            case 5:
                daysInCurrYear += 31;
            case 4:
                daysInCurrYear += 30;
            case 3:
                daysInCurrYear += 31;
            case 2:
                daysInCurrYear += todayIsLeapyear ? 29 : 28;
            case 1:
                daysInCurrYear += 31;
        }

        // calculate the lived (full) days in the year of birth
        int daysInBirthYear = 0;
        switch (birthMonth) {
            case 1:
                daysInBirthYear += 31 - birthDay;
            case 2:
                if (birthYearWasLeap) {
                    daysInBirthYear += (birthMonth == 2) ? (29 - birthDay) : 29;
                } else {
                    daysInBirthYear += (birthMonth == 2) ? (28 - birthDay) : 28;
                }
            case 3:
                daysInBirthYear += (birthMonth == 3) ? (31 - birthDay) : 31;
            case 4:
                daysInBirthYear += (birthMonth == 4) ? (30 - birthDay) : 30;
            case 5:
                daysInBirthYear += (birthMonth == 5) ? (31 - birthDay) : 31;
            case 6:
                daysInBirthYear += (birthMonth == 6) ? (30 - birthDay) : 30;
            case 7:
                daysInBirthYear += (birthMonth == 7) ? (31 - birthDay) : 31;
            case 8:
                daysInBirthYear += (birthMonth == 8) ? (31 - birthDay) : 31;
            case 9:
                daysInBirthYear += (birthMonth == 9) ? (30 - birthDay) : 30;
            case 10:
                daysInBirthYear += (birthMonth == 10) ? (31 - birthDay) : 31;
            case 11:
                daysInBirthYear += (birthMonth == 11) ? (30 - birthDay) : 30;
            case 12:
                daysInBirthYear += (birthMonth == 12) ? (31 - birthDay) : 31;
        }

        // calculate the full lived years since birth
        int fullYearsSinceBirth = TODAY_YEAR - birthYear - 1;
        // ... and considere the leapyears lived
        int leapyearsLived = (fullYearsSinceBirth + birthYearLeapShift) / 4;
        // and calculate the lived days in the lived full years
        int daysInFullYears = fullYearsSinceBirth * 365 + leapyearsLived;

        // add everything together
        int daysLived = daysInBirthYear + daysInFullYears + daysInCurrYear;

        // and finally return the result
        System.out.println("You have been living for " + daysLived 
                + " days now.");
        System.out.println("In addition, this is approximately " 
                + (daysLived / 30) + " full months and " 
                + fullYearsSinceBirth + " full years.");
        System.out.println("Congratulations!");
    }
}
\$\endgroup\$
  • \$\begingroup\$ Do they know loops? \$\endgroup\$ – Hosam Aly May 3 '11 at 11:21
  • \$\begingroup\$ @HosamAly: Yes, they do know loops. \$\endgroup\$ – Torbjörn May 4 '11 at 12:17
7
\$\begingroup\$

The code contains a number of things that would be considered bad style ... and you should not be requiring students to learn/practice bad style. Some of the big problems are:

  • A main method that is 100's of line long is in dire need of refactoring.

  • Drop through in switch statements is considered to be bad style. Certainly, it is rarely used in real applications. You've done it 2 or 3 times.

  • Hard wiring today's date into a program is a terrible idea. Teaching students to hard wire environmental information is a really bad idea.

I'd say that the task that you have set is way too complicated given the restrictions. At the very least, students need to understand static methods and method calls to undertake a task like this. They also need to be told about System.currentTimeMillis().


Given that the problem is set in concrete, and the students' knowledge is as stated, there's not much you can do to improve the readability of the solution, I'm afraid.

The best you could do would be to show them what the ideal solution would look like ... if they were using a larger subset of Java. (And then maybe "fess up" to the fact that the problem was too hard.)


Drop-through switch cases are bad style because drop-through is easy to miss. (And in fact, it is usually an error.) If you really want to use it, you should have a comment at each point where drop through is intended to occur; see http://www.oracle.com/technetwork/java/codeconventions-142311.html#468

I will grant you that yours is an elegant solution ... but a more elegant one would be to use array lookup; e.g.

    final static int[] DAYS_TO_START_OF_MONTH = {
                // Line them up neatly so that it is trivial to manually inspect
                // the constant expressions ...
                0,
                31,
                31 + 28,
                31 + 28 + 31,
                ...
    };

    public int daysToStartOfMonth(int month, boolean isLeapYear) {
          return DAYS_TO_START_OF_MONTH[month] + (month >= 2 && isLeapYear ? 1 : 0);
    }

Teaching people bad programming style is a bad idea period. A lot of students who graduate with a mathematics degree do end up in programming jobs ... and need remedial mentoring to help them unlearn their bad programming habits.

\$\endgroup\$
  • \$\begingroup\$ Thank you for commenting on style issues. That was some of the answers I was waiting for here. The fact, that dropping through switch-statements is bad style is new to me. I thought, that it might be an elegant way of doing this kind of stuff in this case. Though I don't want to teach them bad style, I have to say, that these are mathematics students and most of them will never ever use Java in there live again, but MATLAB, R and SAS. \$\endgroup\$ – Torbjörn Apr 30 '11 at 5:53
  • \$\begingroup\$ Once again, thank you for your suggestions and critical comments. I'll revise my sample code and include your points and finally explain it to my students. Your definition of that final array is a good idea. I'll take your answer as the accepted one -- though I'd like to take all of them as accepted. \$\endgroup\$ – Torbjörn May 4 '11 at 12:26
5
\$\begingroup\$

Wow, arrays would be really nice for this, but I suppose you can give a "rewrite that first problem using arrays" once they learn them. :)

What happens if they enter zero or negative values?

Anyone born in February is automatically invalid:

case 2: //february
    inputOK = (birthDay <= 28);
    inputOK = (birthDay <= 29 && birthYearWasLeap);

This forces inputOk to false when the birth year isn't a leap year, even if birthDay is 5. It should be combined using or logic.

case 2: //february
    inputOK = (birthDay <= 28) || (birthDay <= 29 && birthYearWasLeap);

I would add an error message when the input is not okay. If they know about return or System.exit(1) I would use them after displaying an error message. Better yet, loop until the input is acceptable if they know about looping.

\$\endgroup\$
  • \$\begingroup\$ Oh, yes. I totally missed the logical error of these two lines. Thanks a lot for pointing me on this! I must admit, that I really forgot the last error message (wrong day of birth) and haven't caught input <= 0. \$\endgroup\$ – Torbjörn Apr 30 '11 at 5:43
4
\$\begingroup\$

I would hardly agree to give such an exercise to first-year students at the knowledge level you describe. That's not how people love programming. But anyway, here are some suggestions:

  • You can check the validity of the birth date in a much easier way:

    if (month == 2) {
        inputOK = inputOK && (birthDay <= 28 ||
                              birthYearWasLeap && birthDay <= 29);
    } else if (month == 1 || month == 3 || month == 5 || month == 7 ||
               month == 8 || month == 10 || month == 12) {
        inputOK = inputOK && (birthDay <= 31);
    } else if (month <= 12) {
        inputOK = inputOK && (birthDay <= 30);
    } else {
        inputOK = false; // month > 12
    }
    

    or using switch:

    int daysInMonth;
    switch (month) {
        case  2:
            daysInMonth = (birthYearWasLeap ? 29 : 28);
            break;
        case  1:
        case  3:
        case  5:
        case  7:
        case  8:
        case 10:
        case 12:
            daysInMonth = 31;
            break;
        case  4:
        case  6:
        case  9:
        case 11:
            daysInMonth = 30;
            break;
        default: // invalid month
            daysInMonth = -1; // error value
            break;
    }
    
    inputOK = (birthDay <= daysInMonth); // fails if daysInMonth == -1
    
\$\endgroup\$
  • \$\begingroup\$ Doing it with a loop was one of my initial ideas, however I've chosen the switch-statement. Your cleaned switch is probably more readable than mine. Thank you for this. \$\endgroup\$ – Torbjörn May 4 '11 at 12:24
4
\$\begingroup\$

Given the restrictions of the problem, it looks a reasonable, but could definitely do with some improving:

  • Are you sure none of the students have ever programmed before? Or may have asked for help from someone who has programmed before? Most of the maths students I was at university with knew at least a little bit of programming, and most went on to do jobs involving programming. That's less likely if this is an introductory course, but you want to make sure that if any of them HAVE heard about arrays and functions, you make it clear that your example is BAD without being more streamlined.
  • Even without arrays, you can reduce duplication: collect the "case" statements together as Hosam Aly suggests; used named constants for the lengths of the months if they are used more than once
  • Use comments to introduce NOW the things the students should be doing right from the start. Eg. if you DO fall through from a "case" statement, comment every time. Eg. If you hardcode the date, put a "TODO: get date from system" to make it clear that hardcoding the date is bad. Eg. If you don't validate every input, put another "TODO:" in. Etc. This can be valuable in teaching them to work towards a better program, but without it it may encourage them NOT to.
  • Do NOT encourage the students to simplify leap year calculations. You're right, assuming a leap year every four years works, unless someone over 110 uses the program (since 1900, 1800 and 1700 are not leap years), but you should (if you must) explicitly validate and disallow anyone that old, or (preferably) calculate leap years correctly every time even in sample programs. You want to teach people to do it right from the start[1].

[1] I assume you know this as you chose 1904 as the sample leap year, but not everyone reading may know for sure. FWIW, the correct rule is on wikipedia; it is every year divisible by four is a leap year, except that every year divisuble by 100 is NOT a leap year, except that every year divisible by 400 IS a leap year. But if you're not sure, don't take my word for it, CHECK. These people may never write code that needs to handle dates before 1900 or after 2100, but they should still be taught not to make assumptions without checking them clearly, and it will only confuse anyone who has dim or better memories of how to do it.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for these comments. I will revise my sample code and explain it to my students. Your suggestions of the TODO-comments is great. I asked the students and almost none of them have ever done some programming. The leap year rule has been made clear to all students in the task. \$\endgroup\$ – Torbjörn May 4 '11 at 12:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.