CSE > COMP1511 > On Style (1)

a review of a student’s code style from COMP1511
a reply-to-public; written 2017-08-25

I’ve spent a few minutes reading through this code – somewhat longer than an assessor would have to make a style judgement – and much longer writing this commentary. So, as you’ve probably guessed, I’ve got a few remarks on style; here’s a précis, in descending order of how severe a problem it is.

• Your program must be valid ISO C99: functions without prototypes are illegal.
• Functions should be declared top-down.
• Braces should be in “compact” form, or “cuddled” form.
• Every if, else if, and else block should have braces.
• Operators be consistently surrounded by whitespace.
• Functions should be named consistently.
• Variables should be named consistently.
• Your program must be valid ISO C99: hoisted variables are unnecessary.

Let’s go through those.

• Your program must be valid ISO C99 or C11.

  We’ve not really spoken much about the standards that govern the C programming language, or the various different versions of C that exist, but there have been a few, fairly major revisions that changed the behaviour and structure of the language.

  The most recent was ISO 9899:2011, usually referred to as “C11”. Before that, we had ISO 9899:1999, usually referred to as “C99”. Before that, we had ISO 9899:1990, which was derived from the ANSI standard X3.159-1989, referred to as “C89” or “C90”, or just “ANSI C”.

  And, before that, we had “traditional C”, sometimes known as K&R C, for the authors, Brian Kernighan and Dennis Ritchie. K&R C was first described in the early 1970s, and was extremely lax in its description of language features, like function prototypes.

  In “traditional” C, all functions were assumed to return int, and all function parameters were assumed to be int. This behaviour still exists, albeit only vestigially, in most modern C compilers, and is the source of the wonderfully unhelpful “implicit declaration of function is invalid in C99” message.

  In all standardised versions of C, though, functions and function parameters must have declared types.

  So, why am I raving about this?

   [   151]    int add(h1,m1,h2,m2)//Converting time into minutes and hours
   [   152]    {
  

  This is forbidden. You instead want, with the comment on the preceding line, and the brace on the same line (see below):

   // Converting time into minutes and hours
   int add (int h1, int m1, int h2, int m2) {
  

  While we’re on the topic of functions:

• Functions should be declared top-down.

  This means you must forward-declare your functions. At the top of the file, we have the following:

   [    62]    // =========== Function Prototypes ===========
   [    63]    //       [DO NOT CHANGE THIS PROTOTYPE!]
   [    64]    int getLocalTime (int city, int day, int month, int timeUTC);
  

  You should add your function prototypes up here. And, after the main function, you should have the getLocalTime function, then the functions it calls, after the declaration:

   [   146]    // Converts the time to local time
   [   147]    // [DO NOT CHANGE THIS PROTOTYPE!]
  

  Ideally, you should order your functions so the most abstract function, getLocalTime, is at the top of this section. This should be followed by functions incrementally less abstract: isDST, isDSTNZ, and convert, and then add.

• Braces should be in “compact” form, or “cuddled” form.

  Summarily, this is bad style – I’ve arbitrarily chosen a section from your code, but this is pervasive.

   [   187]        if((month==4 && day<=2 && timeUTC<=300)||(month<4))
   [   188]        {
   [   189]            check=1; //daylight
   [   190]        }
   [   191]        else if((month==9 && day>=24 && timeUTC>=200)||(month>9))
   [   192]        {
   [   193]            check=1;
   [   194]        }
   [   195]        else
   [   196]            check=0;//standard
  

  Instead:

   if((month==4 && day<=2 && timeUTC<=300)||(month<4)) {
       check=1; //daylight
   } else if((month==9 && day>=24 && timeUTC>=200)||(month>9)) {
       check=1;
   } else
       check=0;//standard
  

  Oh, and:

• Every if, else if, and else block should have braces.

  So, that if/else if/else block above should have braces around its else condition, so:

   if((month==4 && day<=2 && timeUTC<=300)||(month<4)) {
       check=1; //daylight
   } else if((month==9 && day>=24 && timeUTC>=200)||(month>9)) {
       check=1;
   } else {
       check=0;//standard
   }
  

  I’m very deeply concerned by this and the preceding style violation, as these are totally different in style to anything we teach in this course.

• Operators be consistently surrounded by whitespace.

  All operators that take two parameters (the “binary” operators) should have whitespace on both sides. There’s only one exception to this rule, the -> operator, and we won’t see it for another week.

   if ((month == 4 && day <= 2 && timeUTC <= 300) || (month < 4)) {
       check = 1; //daylight
   } else if ((month == 9 && day >= 24 && timeUTC >= 200) || (month > 9)) {
       check = 1;
   } else {
       check = 0;//standard
   }
  

  And if you say “oh no, but that makes my lines too long” – rethink your conditions. Add another level of functions in here to handle these particular conditions, if you want.

  The consistency part of this rule is just as important. Later on, we have:

   [   259]            dors=IsDSTNZ(day, month ,timeUTC);
   [   260]            if(dors==0)
   [   261]                convert(timeUTC,TIMEZONE_NZST);
  

  This isn’t consistent at all – there should be whitespace to the right of the comma, not to the left (on line 259)! The whitespace should always be there (on line 261)! Better:

   dors = isDSTNZ (day, month, timeUTC);
   if (dors == 0) {
       convert (timeUTC, TIMEZONE_NZST);
   }
  

  (And, of course, I’ve added braces around that if statement.)

• Variables should be named consistently.

• Functions should be named consistently.

  The style guide dictates the naming convention for variables and functions: descriptive names, in lowerCamelCase. So, variables with names like check, dors, and UTCminutes are violations of this rule, as are functions with names like IsDSTNZ.

  Better names would be, for example, isDaylightSavingsNZ.

• Your program must be valid ISO C99: hoisted variables are unnecessary.

  While we’re discussing variables, it’s worth noting that, as of C99, you don’t have to hoist variable declarations up to the top of the function that declares them, so moving dors in getLocalTime to the top of the function isn’t necessarily valuable.

  Instead, each new “scope” is allowed to introduce its own new names, so you could perfectly validly take a declaration like:

   [   228]            dors=IsDST(day, month ,timeUTC);
  

  … and make it look more like:

   int dors = isDST (day, month, timeUTC);
  

  The name dors won’t escape from the scope it’s defined in, so you can reuse it in other blocks with no issues.

And finally, some logic errors: you throw away, consistently, values moving up and down the call stack.

A function can be treated like a black box that, when given a set of values, produces a result. So, if I were to call getLocalTime, I’d have something that looks like

                                   ┌──────────────┐
(CITY_CHRISTCHURCH, 15, 5,  859) → │ getLocalTime │ → 2059
                                   └──────────────┘

That’s just a specific case, of course; in general, functions tend to look a bit more like this:

                       ┌──────────────┐
(int, int, int, int) → │ getLocalTime │ → int
                       └──────────────┘

When given four int-type values, this function produces an int type value. You don’t have to worry about how it happens, only that it does happen. This is part of what the prototype of a function does for us: it tells the compiler the shape of these values moving through the system.

For each level of abstraction you drill down, you should be able to treat the functions at the next level down as black boxes, too.

             ┌─────────┐
(int, int) → │ convert │ → int
             └─────────┘

Unfortunately, convert won’t behave like this: this function won’t return me a useful int-typed value, because, if I throw away the portions of the function that don’t directly produce me a value:

[   171]    int convert(int UTCtime,int Timezone)
[   172]    {
[   179]        return 0;
[   180]    }

This doesn’t seem like a particularly useful function. For all values I can put into it:

             ┌─────────┐
(int, int) → │ convert │ → 0
             └─────────┘

Worse, if I take away the portions of the getLocalTime function that don’t produce a useful value:

[   220]    int getLocalTime (int city, int day, int month, int timeUTC)
[   221]    {
[   267]    }

There’s nothing left!

Imagine you’re trying to do maths, but the numbers you’re working with, and the working space on which you can write, are balloons filled with helium. You can’t lose a string tying one of these balloons down, otherwise the numbers you’re working with just fly away!

A useful rule to follow: when you transform a value, whether it’s by applying the basic arithmetic operators, or by applying a function, you should always capture the result, or it flies away.

In this case, the return is undefined!

                       ┌──────────────┐
(int, int, int, int) → │ getLocalTime │ → ????
                       └──────────────┘

… so none of the asserts will ever pass, because there’s no value that comes back out of the function. If I just returned every convert:

                       ┌──────────────┐
(int, int, int, int) → │ getLocalTime │ → 0
                       └──────────────┘

If I made convert return the value of the add:

                       ┌──────────────┐
(int, int, int, int) → │ getLocalTime │ → int
                       └──────────────┘

And now your solution might work!