Title: Student Code Critique Competition 13

// From Borland C++ 4.5 page 100 Demonstration 
// of loops using Newtons Method of finding 
// square roots
#include <iostream.h>
const double TOLERANCE = 1.0e-7;
double abs(double x) {
  return (x >= 0) ? x : -x;
}
double sqroot(double x) {
  double guess = x /2;
  do {
    guess = (guess + x / guess) / 2;
  }while(abs(guess * guess - x ) > TOLERANCE);
  return guess;
}
double getNumber() {
  double x;
  do {
    cout << "Enter a number:  ";
    cin >> x;
  } while (x < 0);
  return x;
}
main(){
  char c;
  double x, y;
  do {
    x = getNumber();
    y = sqroot(x);
    cout << "Sqrt ("  << x <<  ") = "
         << y << endl
      << "Enter another number?  (Y/N)  ";
      cin >> c;
      cout << endl;
  }  while (c == 'Y'  ||  c ==  'y' ) ;
  return 0;
}

As Francis had very few entries for this competition he has persuaded me to provide an entry. It is under the strict condition that I must not be the winner. Some of my comments may be a little unfair because I think the material was extracted from an older book, however I am going to pretend for the sake of this review that it was written yesterday. I will stick to my usual format of working through the code as it was provided.

#include <iostream.h>

Well we can argue about this one. There is nothing wrong with that as a header file as long as the author does not claim that it is a Standard C++ header. It is not and never has been. The correct header file for Standard C++ i/o is iostream (note: no '.h') which places everything in namespace std. To keep things simple I will assume that the actual first two lines were:

#include <iostream>
using namespace std;

There is nothing exactly wrong about this next line:

const double TOLERANCE = 1.0e-7;

but there are two things that I would do differently. I never use identifiers that are devoid of lowercase letters if they are not provided by the pre-processor (I never use lowercase letters in preprocessor identifiers). I also try to stick firmly to writing 'const directly after the type being qualified. Neither of these things is a major issue so do not waste time arguing about it.

double const tolerance = 1.0e-7;

However that tolerance seems somewhat generous if you are computing with doubles. It also suffers from being an absolute value which is hardly useful if you are calculating square roots of smallish values. That is a serious fault because it makes a quite unwarranted assumption.

This next function caused me considerable surprise. What is it for? Yes I know what it does, but why has the author written it. Only someone who was substantially ignorant of what the C Standard Library provides would waste time writing something like thi (particularly with a reserved name in C).

double abs(double x) {
  return (x >= 0) ? x : -x;
}

So those three lines should be replaced by:

#include <cmath>
// Use double fabs(double) from the library

Now the next bit looks OK though we will have to patch it up to handle the poor choice of the value for tolerance and to replace abs with fabs.

double sqroot(double x) {

Note that the writer assumes that x will be positive. We need to add a line:

if(x < 0) throw range_error(
"cannot find square root of negative number")

Which means we will also need to add:

#include <exception>
  double guess = x/2;

and we better add something to mend that tolerance calculation by scaling it to the actual value of x:

double const tolerance = ::tolerance*x;

Yes, you are right, I am being lazy and should really use another name, but it reminds people of the way we can use a local version of such a value which depends on the global one.

  do {
    guess = (guess + x / guess) / 2;
  }while(abs(guess * guess - x ) > TOLERANCE);

Patching that line gives us:

} while (fabs(guess * guess - x) > tolerance);
  return guess;
}

I assume that the rest of the code is a test harness to demonstrate that the above function works. I hope that you hate functions that continue for ever without giving you a hint that you are not providing what you want. Imagine the poor user who enters -1. It is a perfectly good number and nowhere has this function stated that it only accepts positive floating point numbers. Actually, I consider this sort of function the kind of utility function that will handle the other kind of input error where the user types something like "minus one". Now the function can never end.

double getNumber() {
  double x;
  do {
    cout << "Enter a number:  ";
    cin >> x;
  } while (x < 0);
  return x;
}

The above function should be replaced with something like:

double getdouble(char const * message){
  int retries 3;
  while(retries--){
    cout << message ;
    cin >> x;
    if(!cin) {
      cout << "invalid input found \n";
      while(cin.get() != '\n');
      cin.clear();
    }
    else return x;
  }
  throw exception("repeated invalid input from console")
}

A bit more polish would be nice but the above should give you a fair start.

main(){
  char c;
  double x, y;
  do {
    x = getNumber();
    y = sqroot(x);
    cout << "Sqrt ("  << x <<  ") = "
         << y << endl
      << "Enter another number?  (Y/N)  ";
      cin >> c;
      cout << endl;
  }  while (c == 'Y'  ||  c ==  'y' ) ;
  return 0;
}

Well this is what I would write to achieve more or less the above test functionality. First we need an input function to deal with the yes/no question and ensure the input stream is left in a clean state:

bool yesno(char const * message)
  cout<< message;
  char c = cin.get();
// flush the input
  while(cin.get() != '\n');
  return(c=='Y' || c=='y');
}
int main (){ // note the original forgot 'int'
  try {
    do {
      double x = getdouble(
            "Type in a positive number");
      if(x,0){
        cout<< "A positive number,"
            << " input ignored \n";
        continue;
      }
      double y = sqroot(x);
      cout << "Sqrt ("  << x <<  ") = "
           << y << endl;
    while (yesno(
        "Enter another number? (Y/N): "));
  }
  catch(exception e){ cout << e.what(); }
  return 0;
}

There are quite a few rough edges in my code that could be further smoothed. For example I have twice flushed the input stream, it would be worth writing a simple inline function to do this. However I think that this code is a great improvement on the original though it does not take much more space.

I suspect the lack of entries to this competition is because many did not see the faults. Training your eye to recognise poor code is a valuable skill. If you did not see at least half the flaws in the above code then you need to hone your code inspection skills, you are too trusting of published code.