Title: Student Code Critique Competition

It was reported that the program did not collect the second number after the operator symbol had been entered. My own observation was that it

I suggest that the original user was probably typing the operation symbol in response to the "second number" prompt, not having noticed that the "operation" input had been skipped. In that case the observed behaviour would have seemed to be as reported.

The first call to printf prompts the user to enter the first number. The user types in the number and presses the "Enter" key, thereby placing a string of characters into the input stream such as:

1 2 . 3 4 5 \n

The scanf function reads the input stream. The format specifier "%e" tells scanf that it is looking for a floating-point number, but does not specify the number of characters to be read. scanf therefore keeps reading characters as long as they are plausibly part of a floating-point number. The resulting value is assigned to num1. The new-line character, \n, corresponding to the Enter-key, cannot be part of a floating-point number and so is not consumed by scanf. It is not discarded either; it stays in the input stream waiting to be read. The next call to printf prompts the user for the operation symbol. scanf again reads the input stream. This time the format specifier is "%c", which differs from other format specifiers in these respects:-

Now, there is already a \n character in the input stream, left over from the input of num1. scanf reads this straight away and assigns it to operation. The user doesn't get a chance to type in the actual operation symbol before...

The next call to printf prompts the user for the second number, num2. By now the input stream is empty, so scanf has to wait for the user to type in a new string of characters, which are read in and assigned to num2 in the same way as for num1.

The program can be made to work almost as expected by inserting a call to getchar immediately after the first scanf.

scanf("%le", &num1);  getchar();

This getchar "swallows" the \n at the end of the input stream, ensuring that the second scanf has to wait for the user to type in an operation symbol. It does not matter that this character will also be followed by \n, because the third scanf, under the control of format specifier "%e", will skip over any leading whitespace characters, including \n. Incidentally, my compiler (Borland Turbo C++ version 3.1 for Windows) required the format specifier "%le" (that's a lowercase 'L') in order to assign correctly to a variable of type double.

Another correction needs to be made before the program will work properly. As they stand, the two if-statements are rendered ineffective by the semi-colon immediately following the test:

if (num2 == 0);

This causes the value of num2 to be compared with zero, but nothing in particular is done on the basis of the result. The program continues with the next statement regardless. Consequently, if the division operation is selected, the program both prints the error message and attempts the calculation, whether num2 is zero or not. This is easily rectified:

if (num2 == 0)
   printf("ERROR: DIVIDE BY ZERO!!\n");
else {
   total = num1/num2;
   printf("%e\n", total);
}

The program is written in a style which is only suitable for very short, simple programs. After the input section, the switch-statement sends the thread of execution down one of four possible paths. These branches are quite short, between four and eight statements long, so the overall structure of this program is still quite easy to see. But suppose there were more branches, and each was forty or more lines in length. It would be much more difficult to see what was going on then.

As a result of adopting such a style, there has been some unnecessary repetition. For example, each branch has at least one printf statement for printing out the answer, but they are all identical. The two printf statements for printing the error messages are also similar to one another. If it became necessary to change the format of the output; all these statements would have to be changed. They are all quite close together here, so its not an onerous task. But again suppose that the switch had more branches and they were much longer; you could spend hours tracking down all the occurrences of printf and deciding which ones had to be changed, (and you would still to miss one).

First, I broke the problem down into a series of simpler steps, each of which can be expressed as a function:-

input(&num1, &op, &num2);
answer = evaluate(num1, op, num2);
output(answer);

I then considered the types of each of the variables involved. It would be sensible for num1 and num2 to be floating point numbers, i.e. of type float or double, and for the operator op to be a char. What about the answer? If all went well it too would be a floating-point number. But it could also be an error message, that is, a string of characters. I therefore decided to define a data structure, Answer, which could accommodate both a double value and a pointer to a char, but which could still be passed around as a single object. Consequently, the "top level" of my program looks like this:-

#include <stdio.h>
struct Answer {
   double num;
   char   *err;
};
int input(double *ptr_num1, char *ptr_op,
        double *ptr_num2);
struct Answer evaluate(double num1, char op,
                 double num2);
struct Answer divide(double num1,double num2);
void output(struct Answer answer);
int main(){
  double    num1, num2;
  char      op;
  struct Answer answer;
  input(&num1, &op, &num2);
  answer = evaluate(num1, op, num2);
  output(answer);
  return 0;
}

In my input function, rather than making the user type in the two numbers and the operator on separate lines, I have gone for a single scanf statement, which allows a more natural expression like "2.78 + 4.65" to be typed in. I have used "%1s" (a one-character string) instead of "%c" (a single character) in the format specifier for the op variable. The difference may seem subtle, but the advantage of "%1s" is that any spaces preceding it will be ignored, and the first non-space character will be assigned to op. This gives the user the option of including spaces.

Note that the parameters to the input function, like those of scanf, are pointers, i.e. variables containing the addresses of other objects. This allows the values of the variables in the main function, to which they point, to be changed. Because these objects are pointers, they do not need to be preceded by '&' in the call to scanf.

int input(double *ptr_num1, char *ptr_op,
      double *ptr_num2){
  printf("> ");
  scanf("%le %1s %le", ptr_num1, ptr_op,
           ptr_num2);
  return 0;
}

The evaluate function starts off by defining its own local Answer structure. The numeric component, answer.num, is initialised to 0 and the pointer component, answer.err, is initialised to NULL (pointing at nothing). I have then used a switch statement to handle the various possible values of op. The add, subtract and multiply cases assign new values to the num component of answer, leaving the err component as NULL. The default case makes the err component point to the start of a suitable error message, leaving the num component as zero. Because the divide operation is non-trivial, I have hived it off to a function of its own, which returns a complete Answer structure. Finally, the resulting answer is passed back to the main function.

struct Answer evaluate(double num1, char op,
                       double num2) {
  struct Answer answer = {0, NULL};
  switch(op) {
    case '+':
      answer.num = num1 + num2;
      break;
    case '-':
      answer.num = num1 - num2;
      break;
    case '*':
      answer.num = num1 * num2;
      break;
    case '/':
      answer = divide(num1, num2);
      break;
    default:
      answer.err = "Unknown operator";
      break;
  }
  return answer;
}

The divide function also defines its own local Answer structure, its components initialised to 0 and NULL. If num2 is not zero, the result of the division operation is assigned to the numeric component. Otherwise, the pointer component is made to point to the start of a suitable error message. The resulting answer is returned to the calling function.

struct Answer divide(double num1, double num2) {
  struct Answer answer = {0, NULL};
  if (num2 != 0.0) answer.num = num1 / num2;
  else  answer.err = "Division by zero";
  return answer;
}

The output function takes an Answer structure as its parameter. If the err component is NULL, then the computation must have succeeded without any errors, so the num component is output. Otherwise, the character string pointed to by err is output.

void output(struct Answer answer) {
  if (answer.err == NULL) printf("= %le\n", answer.num);
  else printf("ERROR: %s\n", answer.err);
  return;
}