Title: Student Code Critique Competition 31

Prizes provided by Blackwells Bookshops & Addison-Wesley

Please note that participation in this competition is open to all members. The title reflects the fact that the code used is normally provided by a student as part of their course work.

This item is part of the Dialogue section of C Vu, which is intended to designate it as an item where reader interaction is particularly important. Readers' comments and criticisms of published entries are always welcome.

Have you ever come across a tricky bug at work that took you the whole day to find, or an exercise at school that didn't work the way you expected to? Those could be good opportunities not only to share it with other members, but to receive feedback from them. After all, this belongs to the Dialogue section, so who better than you to take part?

Remember that you can get the current problem set on the ACCU website (http://www.accu.org/journals/). This is aimed at people living overseas who get the magazine much later than members in the UK and Europe.

Here is a program Francis collected which is riddled with poor design, naming, etc. as well as the actual problem:

I'm getting a "parse error before else" at the line indicated by the arrow

void IS_IT_A_DDR(string& mtgrec,
                 string& temprec,int& ddrrc) {
  string Day2="SunMonTueWedThuFriSat";
  string Daytoken="0123456";
  int badday=0;
  if (mtgrec.size() < 8) {
    ddrrc=0;
    return;
  }
  for (int i=0; i <= 6; i++) {
    if (mtgrec.substr(0,3)
                == Day2.substr((i+1)*3-3,3)) {
      if ((mtgrec.substr(3,1) == "0")
             || (mtgrec.substr(3,1) == "1")) {
        if ((mtgrec.substr(7,1)).
          find_first_of("BCLMOPSTW*") != -1) {
      temprec=Daytoken.substr(i,1)
                           + mtgrec.substr(1);
      ddrrc=1;
      return;
      }
        else {
      ddrrc=2;
      return;
      }
    else { <<< compiler diagnostic
      ddrrc=3;
      return;
    }
    }
  }
  else badday++;
  }
  if (badday == 7) {
    ddrrc=4;
    return;
  }
  else ddrrc=5;
  return;
}

if((mtgrec.substr(3,1) == "0")
   || (mtgrec.substr(3,1) == "1"))

const string mtgDay = mtgrec.substr(0, 3);

The editor's choice is:

Neil Youngman

Please email <francis@robinton.demon.co.uk> to arrange for your prize.

My first reaction to the student's question is 'Are you surprised that the code has an error?' I would rapidly follow it up with 'If the corrected code passes the compiler, would you trust it?' I think that the only acceptable answer to both these questions is 'no'.

My next question is 'What should you do about it?' I would try to guide the student into 'Redesign the code and factor the separate actions into their own informatively named functions.' If performance becomes an issue after doing that, it is time to consider helping the compiler with the inline keyword.

Most of the reorganisation I want to do is concerned with the implementation so I will move that out into the unnamed namespace.

Before I do any of that, I take strong objection to both the function name (spelt in all uppercase) and the function return type. Any function whose name asks a question should return a bool. However it seems to me that the function does not answer a simple binary question but asks something else for which there answer is a choice of five things. I have no idea what DDR means in this context (I am pretty sure it does not refer to 'Dance Dance Revolution', 'Developers Diversified Reality' nor to some form of SDRAM), nor what the classification stored in an out-parameter (ddrrc) means so I will have to use some placeholders. These placeholders should be replaced with meaningful names. enums are designed to deal with this kind of issue. As the enumeration constants need to be available in multiple translation units, it needs to be declared in a header file. Let me start with that:

#ifndef DDR_DECLARATIONS_H
#define DDR_DECLARATIONS_H
#include <string>

enum ddr_classification{
  ok, too_short, bad_day, bad_flag,
  bad_symbol, err5
};
ddr_classification classify(
      std::string const & mtgrec,
      std::string & outrec);
inline 
void IS_IT_A_DDR(std::string const & mtrec,
                 std::string & temprec,
                 int & ddrrc) {
  ddrrc = classify(mtrec, temprec)+1;
}
#endif

I have provided a simple forwarding function to provide temporary support for the ill-named function so that no immediate changes need to be made to code elsewhere. I would expect that to be rapidly replaced. I have made ok take a zero value so that in future it will be possible to use the return value of classify() for a rapid check of validity. Note that I use fully qualified names in the header file and that I have added a const qualifier to the first parameter.

There is also the question of those two string variables; they aren't variable nor are the local (though they could be static). Such items belong in the associated unnamed namespace.

Here is the start for the implementation file:

#include "ddr_declarations.h"
using std::string;
namespace {
  string const daynames[] = {"Sun", "Mon",
                          "Tue", "Wed", "Thu",
                          "Fri", "Sat"};
  char * const daynumber = "0123456";
  char * const symbols = "BCLMOPSTW*";
}

I would favour a better name for symbols but without knowing the context that is the best I can do. Now let me try to write a halfway sensible definition for classify().

ddr_classification classify(
      std::string const & mtgrec,
      std::string & outrec){
  if(mtgrec.size() < 8) return too_short;
  int daynum(dayname_to_int(mtgetrec));
  if(daynum > 6) return bad_day;
  if(not valid_flag(mtgrec.substr(3, 1))) 
            return bad_flag;
  if(valid_symbol(mtgrec.substr(7, 1))){
    outrec = daynumber[daynum] 
                  + mtgrec.substr(1);
    return ok;
  }
  else return bad_symbol;
}

Now notice that if the original code's for-loop ever exited that the student's badday variable must be equal to 7. There just isn't any other way through that nest of if's. That was far from obvious in the original. Separating out the various conditions and only continuing if everything is still checking out leads to much clearer code. It also much better models the way would handle the problem ourselves. First check that the first three characters represent an abbreviation for a day, next check that the fourth symbol is acceptable then check that the eighth one is OK. Human beings might only notice that there were too few symbols at that last stage, though it is easier to check it first from a program perspective.

Now let me go back and add the requisite helper functions (which will go in the unnamed namespace).

int dayname_to_int(string const & mtgetrec){
  for(int i(0); i != 7; ++i){
    if(mtgrec.substr(0, 3) == daynames[i]{
      return i;
    }
  }
  return 7;
}

Yes, I know there is a magic number lurking in there, but I am running short of time if David is to get this in time to use it.

bool valid_flag(char flag){
  if(flag == '0') return true;
  if(flag == '1') return true;
  return false;
}
bool valid_symbol(char symbol){
  for(int i(0); i != strlen(symbols); ++i){
    if(symbol == symbols[i])
      return true;
  }
  return false;
}

Now I think that this code represents the intention of that provided by the student. It would have been much easier had the student specified what the code was intended to do. Notice that the coding error was a direct consequence of an inappropriate view of how to code the problem. The student was willing to use all kinds of tools he had found in the Standard C++ Library but the tool he really needed was his own brain. Sadly once the code was working too many instructors would accept it.

Remember that this is part of the Dialogue section of C Vu so you have an implicit invitation to critique my solution as well as add any other useful information you have about the actual problem.

(Submissions to <scc@accu.org> by January 10th)

Here is the code I have using the equation to drop the lowest number from the grades. The problem is, if I change up number 3 and number 4, I get a different answer. I used the numbers 80, 84, 60, 100 and 90. Putting the numbers in like that, I get 88 but, if I mix up the 100 and 60 then I get a grade of 81. Can anyone tell me why it is not finding the lowest number and just dropping it when I tell it to (- lowest)?

#include <iostream>
#include <iomanip>

using namespace std;

int main() {
int test1, test2, test3, test4,test5,average,
                          averagescore,divide;

cout <<"This program will gather five test
                                scores and\n";
cout <<"drop the lowest score, giving you the
                                   average\n";
cout <<"\n";
cout <<"Please enter Five Test scores\n";
cin >> test1>>test2>>test3>>test4>>test5;

int lowest = test1;
       // test 1 is the lowest number unless
if (test2 < test1)lowest = test2;
       // test 2 is smaller than test 1 unless
if (test3 < test2)lowest = test3;
       // test 3 is smaller than test 2 unless
if (test4 < test3)lowest = test4;
       // test 4 is smaller than test 3 unless
if (test5 < test4)lowest = test5;
       // test 5 is smaller than test 4.

average = (test1+test2+test3+test4+test5);
       // all test scores averaged together
averagescore = average - lowest;
       // average score minus the lowest grade
divide = averagescore /4;
       // average grade is then divided by 4
cout << divide<< endl;
       // final grade after division
return 0;
}

Besides the question asked by the student, this code gives you a chance to discuss topics such as extensibility, design and style. Please address as many issues as you consider necessary, without omitting the answer to the original question.