Gloucester County High School Programming Contest

Normally, programs at our contests are judged by four people in each of four categories, across two language categories. This year's contest was run with a skeleton crew of only two judges, each of whom took on two categories, and each of whom judged every program in every language. This provided the judges with a more detailed insight into the patterns which appeared in the submitted programs. The judges have offered the following remarks based on what they saw:

The sample output as defined needs no addition. Many programs had opening paragraphs explaining function, or printed menus of the signals, or closings that say "Buh-bye now!"
The program's sample output as depicted on the PDF had none of those things, and thus they were not necessary. Every second spent typing that in is a second spent not solving the problem. There is nothing wrong with such friendly additions to software; but in the context where a specific output has been provided, it's best to stick to the spec. Don't do more work than is required.
Some students should be reminded in detail about incremental development. We got programs that looked really really good, but wouldn't compile because of typographical errors. Visual C++ sometimes does a poor job identifying the point of error in certain cases (a missing semicolon can produce an error message hundreds of lines away from the actual problem).
In all cases, the smart thing is to write one part of the program at a time (possibly several functions), and ensure that it compiles and runs before going on. That way, if the program won't compile, you only have 50 or 60 lines to look through, instead of (as was the case with one student) 350.
A program which just read in and printed the lengths of the CD-ROM tracks scored higher than one which did everything but wouldn't compile and thus had no output.
Remember that the programs are printed and examined by the judges on paper. Many students maximize windows to type in them (sort of making pointless the idea of having windows at all), and the result was comments and lines of code better than 118 characters wide. When printed at 80 columns on USLetter-size paper, the result was lots of wrapping. Points weren't taken off, but that sort of thing takes longer to read and understand.
If you aren't using an 80-column window, one solution is to put something like this in your code:
```
//3456789012345678901234567890123456789012345678901234567890123456789012345
```
and then make sure all your lines are less than 75 charaters. Before submitting the program, delete the comment so it doesn't clutter up the printout.
This didn't effect the outcome of the contest, but if we'd needed a tie-breaker in "readability" this would have been one thing used.
Also: when breaking lines, break and indent on logical meaning:
Not like this:
```
      System.out.print(currentState + " track " + currentTrack +
      " " + currentTrackTime);
   
```
but this:
```
      System.out.print(currentState +
                       " track " + currentTrack +
                       " " + currentTrackTime);
   
```
Don't repeat nontrivial code. The Java print listed above (along with some other lines) appears in that program 10 times. A better solution would be to have a function printPlayerState, which accepts the relevant arguments and prints out the state of the CD player. It would make the program shorter, improve the score in `modularity', and also make it easier to fine-tune the output. If you decide something is wrong, then you only have to fix it in the function, instead of changing it in ten places (and testing to make sure you didn't miss any).
Don't make functions of trivial code which isn't repeated. Consider this function:
```
        int getnumsignal(){
            //here is our number of singals
            int ns;

            //get that from the user
            cin >> ns;

            //and return it
            return ns;
        }
```
Getting the number of signals happens once every run. What we have here is a 10-line function which doesn't get us anything. Since the one-line call `numsignals = getnumsignal();' only replaces the one-line `cin >> numsignals;', it doesn't even make the main routine any shorter.
Keep related program subtasks together. Several programs had functions like get_number_of_tracks, which only read in and returned one number. Not only does this repeat the problem mentioned above, there's a problem of breaking the job down by logical units: why not read the number of tracks in the same place you read in the tracks themselves? They go together logically, and so should go together in the code. The person who wrote a sample solution in C did this in the main routine:
```
        /* Read lengths of tracks */
        scanf("%d", &numTracks);
        for (tracknum = 1; tracknum <= numTracks; tracknum++)
                scanf("%d", &cd[tracknum]);
```
It's only three lines, so doesn't strictly require its own function. The sample Ada solution puts it in a function (the CD information in this version is a record with several fields):
```
      ------------------
      -- Read_CD_Data -- Read data about tracks on CD
      ------------------
      procedure Read_CD_Data (Disc : in out CD_Info_Type) is
      begin
         Get(Disc.Tracks);
         for Track in 1..Disc.Tracks loop
            Get(Disc.Time(Track));
         end loop;
      end Read_CD_Data;
```
In both cases, reading the number of tracks and reading the lengths of the tracks is done in the same place (not even a blank line between them), instead of being broken into two separate pieces and done in separate locations.
Don't hardcode test data. Read from a file, or the keyboard, or ANYTHING, but don't hard-code data which changes from run to run.
Good comments explain what isn't obvious to another programmer. Anything that the syntax of the language tells you doesn't need to be commented. This, for example:
```
      //function call
      get_steps(track_times,tracks);
```
is what's called "DUH" comment. By adding clutter to the code, this lowers a readability score. (Also, there should be a space after the comma.)
At the other end of the spectrum, there are undocumented things which can only be guessed at, especially magic numbers:
```
      cin.ignore(90, "\n");
```
Why 90? Why not 80, or 100?
One good way to think about comments draws from Aristotle's Four Causes. Like Newton, Aristotle has been passed by, but is still useful for explanations. These were his idea about how to answer `why?' questions. They were: Material, Formal, Proximal (or efficient), and Teleological (or Final). For example, `Why did the barbell roll and dent the wall?'

Material:
because its material is iron, which is heavy and hard, so it dented the wall on impact.
Formal:
because the plates have the form of circles and thus it rolled.
Proximal:
(the nearest cause) because it was pushed and that started it moving. (It was round and heavy before being pushed, so being pushed is the thing closest to making it dent the wall.)
Teleological:
(relating to design or purpose) because I pushed it in the corner to get it out of the way.

The formal and material causes are of no interest in program comments; asking What material is a program made of? doesn't even make any sense.
The proximal cause of something in a program is determined by the syntax of the programming language. Comments don't need to explain the proximal cause except when a language is known to be unfamiliar to the person reading it.
The one we care about is the teleological cause: for what reason does the code work one way instead of another? Imagine that after the contest you are going to show your program to your teacher. What will you say? Will you really point at a line that reads get_steps(track_times,tracks); and say `This line here is a function call.' ? If not, why on Earth would you put in a comment which says the same thing?
Here's a hunk from the Ada sample solution:
```
   ---------------
   -- Constants -- limits from problem description
   ---------------
   Max_Disc_Tracks : constant Integer := 20;
   Max_Track_Time  : constant Integer := 30 * 60; -- 30 mins * 60 secs
```
Later, the program uses `Max_Disc_Tracks' when the array is declared, instead of using the `20' (this makes it easy to change the program if the number of allowed tracks changes). For the Max_Track_Time, the comment is a bit obvious but magic numbers always need documentation. And where did all these magic numbers come from? They came from the problem description, and it says so.
The tricky part of the code got a really long comment:
```
      -- If the player is stopped or paused (or off), its state won't
      -- change between one signal and the next.  The only thing we have
      -- to worry about is whether it was in play, and has either
      -- finished one track and gone on to the next, or has reached
      -- the end of the disc and stopped playing.
      if Player.Activity = Play then
         [ more stuff deleted ]
```
The author wrote about this:
That's a note to ME, because at first this function didn't have any code in it. It was called "Update_Status" and did nothing, having been written just so the code could be compiled to test whether "Show_Status" worked. When I got back to it later, this was a reminder of my thoughts at the design stage. Most of the time, design-time comments get deleted or shortened, but this hunk of code was tricky enough that I thought the comment might be needed later for debugging.
Good variable comments say what a variable is FOR. Don't do things like this:
```
      int count; // counter variable
```
You don't have to say that it's a variable; that's sort of obvious (what with you declaring it and all). You don't have to say it's a counter; that's obvious from the name. But what the heck does it count?
On which note, consider this one:
```
        //declair cd tracks
        const int cd_tracks=gettracks();  //then delair an int array that size
        //track time
        int track_time[20]={0};
```
Not only is this obviously proximal, but it misspells `declare' and doesn't even mention that the variable is being assigned in addition to being declared. Note also that the second comment is wrong: track_time is not declared the size of cd_tracks: it's set to size 20.
With the assignment all the way up here at the top of the declarations, it's easy to miss seeing it. Of course, making it const requires that it be assigned at declaration. Const is nice, but probably better to just declare the thing and then do the assignment later on, right before reading in the tracks. (See above comment about putting related things together.)

Good comments are neatly formatted. If space allows, variable comments should go on the same line. Note the DUH comments in the next bit: the array track_time has a comment which says only `track time'. Also, consider the variable name signalnumber. That suggests it's a loop index, as in `Now we'll do signal number 2'. How about signalcount, or numsignals instead? And it seems a bad idea to have both "tracktime" and "track_time" declared in the same context; that's an invitation to mixing them up. Instead of this:


        //track time
        int track_time[20]={0};
        //number of signals
        int signalnumber=0;
        //track time struct for minutes and seconds
        TIME tracktime;
        //array to store signals
        SIGNAL signal[100];
        //cd player struct
        CD cd;
        //play flag
        bool play=false;
        //time struct to track song time
        TIME playtime;

without changing the structure of the program or unravelling the tracktime/track_time problem, how about this:

      

      int    track_time[20] = {0}; // length of every track on disc
      TIME   tracktime;            // struct holds time as hrs/mins/secs
      int    numsignals = 0;       // how many signals to process
      SIGNAL signal[100];          // signals read
      CD     cd;                   // Information about CD player
      bool   play = false;         // currently in play mode?
      TIME   playtime;             // how long we've been in play

Isn't that easier to read? And if you're not writing stuff that says `Hi, welcome to my happy program. Won't you be my neighbor?', you'll have time to neaten things up.