One program using three functions to compare two double-precision numbers

Question

I am currently learning C++ though Bjarne Stroustrup book Programming Principles and Practices using C++.

Going through the drills, I am trying to purposefully write efficient and readable code (good coding practices. However, I think I may be overdoing it with my use of functions for tasks within my program.

//Function to check if the entered numbers are similar in value
double similarValues(double valueBigger, double valueSmaller)
{
    if ((valueBigger - valueSmaller) < 0.01) {
        return 1;   //Return 1 if the values are similar
    }
    else {
        return 0;
    }
}

//Function checks which value is larger
double valueLarger(double value01, double value02)
{
    if (value01 > value02) {
        return value01;
    }
    else if (value01 < value02) {
        return value02;
    }
    else {
        return 0;       //Return 0 if the values are equal!
    }
}

//Function checks which value is smaller
double valueSmaller(double value01, double value02)
{
    if (value01 > value02) {
        return value02;
    }
    else if (value01 < value02) {
        return value01;
    }
    else {
        return 0;       //Return 0 if the values are equal!
    }
}

int main()
{

    // Reads two integers on each loop and prints them//
    double value02; 
    double value01;

    while (cin >> value01 >> value02) {

        // Checks if the functions valueSmaller & valueLarger have not retuned 0
        if (valueSmaller(value01, value02) != 0 || valueLarger(value01, value02) != 0) {
            cout << "The smaller value is: " << valueSmaller(value01, value02) << " , the larger value is: " << valueLarger(value01, value02) << "\n";

            //Checks if the values are at least similar, returns 1 if true
            if (similarValues(valueLarger(value01, value02), valueSmaller(value01, value02)) == 1) {
                cout<<"The numbers are almost equal. \n";
            }
        }
        // If functions valueSmaller & valueLarger have returned 0 then do the below instuction
        else if (value01 == value02) {
            cout << "Both of these numbers are equal!" << "\n";
        }
        // If the values entered cannot be processed then output an error message
        else {
            cout << "Not a value I know! Incorrect input" << "\n";
        }

    }
}

Please critically (I do not mind if it is harsh) state what programming practices I am not following or following correctly. Also, whether my coding is on par with what you would deem as following great coding conventions.

• Am I overdoing the functions?
• Am I using return values differently then they should be used?
• Is my code readable?
• Are my comments accurate and to the point or are they vague?

Answer 1

Your functions and their use are very curious:

• A function to determine if two values are similar should be symmetric in its arguments, and return a bool.
• A function determining the truth of a property should be called is@. similarValue is a bit of a puzzler, which is actually good as the functions behavior is very surprising.
• A conditional expression returns a bool, which can be implicitly converted to an integer of value 0 (false) or 1 (true).
• I'm not sure what I should do with valueSmaller and valueLarger, but I am sure that I don't want to see anything like them ever again.

Other points:

• Seems you also don't know the conditional-operator (expr ? value-if-true : value-if-false). It can be used to write things better, but beware of getting carried away with it.
• You are using using namespace std;.
  That's a bad idea, read "Why is “using namespace std;” considered bad practice?".
• When you output string- and char-literals consecutively, just concatenate them together. Also, if you have a length-1-string-literal, a char-literal can serve the same purpose more efficiently.
• Your comments are, I'm sorry to say, useless, as they just rephrase some of the more obvious code.
  When you use descriptive names, most other comments for describing what code does also become superfluous (exception: doc-comments which will be extracted for bare-bones documentation).
  Comments are for giving non-obvious reasons for doing things you do, reasons for doing them the way you do them, or describing how you do something.
  See this good answer by rolfl about how to properly comment.

Answer 2

I'm just going to break this down by the method.

---

double similarValues(double valueBigger, double valueSmaller)
{
    if ((valueBigger - valueSmaller) < 0.01) {
        return 1;   //Return 1 if the values are similar
    }
    else {
        return 0;
    }
}

Since we're only interested on the truth value result here, we should use a boolean return to decide if the numbers are "similar". This could also be much more flexible and terse. We can use std::abs to achieve this.

//returns true if the variance is less than 0.01
bool valuesAreSimilar(double a, double b) {
  return abs(a - b) < 0.01;
}

---

//Function checks which value is larger
double max(double value01, double value02)
{
    if (value01 > value02) {
        return value01;
    }
    else if (value01 < value02) {
        return value02;
    }
    else {
        return 0;       //Return 0 if the values are equal!
    }
}

//Function checks which value is smaller
double min(double value01, double value02)
{
    if (value01 > value02) {
        return value02;
    }
    else if (value01 < value02) {
        return value01;
    }
    else {
        return 0;       //Return 0 if the values are equal!
    }
}

Right up front, something to think about is that considering the case where the values are equal is probably unnecessary. Keep in mind you can always check if the values are equal outside of the context of these methods (and in nearly every case, this is preferable)(Thank you to Deduplicator for reminding me to address this). These are perhaps good to write for practice, but in a real code base, I recommend using std::min and std::max (credit to @MORTAL). For good recommendations in optimizing these two, check out @Greg Burghardt's answer. I'm changing the signatures to max() and min() for brevity's sake.

---

int main()
{
    double value02; 
    double value01;

    while (cin >> value01 >> value02) {
        if (valueSmaller(value01, value02) != 0 || valueLarger(value01, value02) != 0) {
            cout << "The smaller value is: " << valueSmaller(value01, value02) << " , the larger value is: " << valueLarger(value01, value02) << "\n";
            if (similarValues(valueLarger(value01, value02), valueSmaller(value01, value02)) == 1) {
                cout<<"The numbers are almost equal. \n";
            }
        }
        else if (value01 == value02) {
            cout << "Both of these numbers are equal!" << "\n";
        }
        else {
            cout << "Not a value I know! Incorrect input" << "\n";
        }

    }
}

This one is a little wonky to look at. We can do a bit with it to make it nice by checking if the values are equal first. Another thing to note is that your final else statement can't be reached. If you'd like to do error checking, I recommend doing a stream failure check.

Update: As Ruslan pointed out, the while loop will never be entered if there is an assignment failure, so we'll move the failure message outside the loop.

int main()
{
    double value02; 
    double value01;

    while (cin >> value01 >> value02) {
        if (value01 != value02) {
            cout << "The smaller value is: " << min(value01, value02) 
                << " , the larger value is: " << max(value01, value02) << "\n";
            if (valuesAreSimilar(value01, value02)) {
                cout << "The numbers are almost equal. \n";
            }
        }
        else {
            cout << "Both of these numbers are equal!" << "\n";
        }
    }
    cout << "Not a value I know! Incorrect input" << "\n";
}

---

Your final code could look like this:

#include <iostream>
#include <algorithm>
#include <cmath>

using std::cout;
using std::cin;
using std::min;
using std::max;

//returns true if the variance is less than 0.01
bool valuesAreSimilar(double a, double b) {
  return std::abs(a - b) < 0.01;
}

int main()
{
    double value02; 
    double value01;

    while (cin >> value01 >> value02) {
        if (value01 != value02) {
            cout << "The smaller value is: " << min(value01, value02) 
                << " , the larger value is: " << max(value01, value02) << "\n";
            if (valuesAreSimilar(value01, value02)) {
                cout << "The numbers are almost equal. \n";
            }
        }
        else {
            cout << "Both of these numbers are equal!" << "\n";
        }
    }
    cout << "Not a value I know! Incorrect input" << "\n";
}

Answer 3

• Function similarValues() should be a bool instead of a double and return true or false
• You could probably combine your other two functions into one that returns different things for larger the same or smaller instead of returning the larger type or again use a bool to determine whether a value is larger and return true or false.

• Consider using switch statements instead of if statements for more readable code.

  cout << "The smaller value is: " << valueSmaller(value01, value02) << " ,     the larger value is: " << valueLarger(value01, value02) << "\n";
  

• Instead of using << "\n" consider using << endl

• Other than that your code is reasonably readable, try and keep your comments concise and to the point and make sure you only use doubles when you need to.
• You are probably not overdoing functions although some of them could be combined as I have already said.
• Consider using using std::cout instead of using namespace std or avoid it all together.

Answer 4

This started out as a comment since I'm only going to discuss similarValues(), but it grew a bit big, so here we go.

First of all, I strongly recommend @nukeforum's suggestion that you use the abs() function. Also, as has been mentioned aplenty already, similarValues() should return a bool:

bool similarValues(double a, double b) {
    return abs(a - b) < 0.01;
}

Now, about that 0.01... you're using 0.01 as the epsilon value here. It's kind of a magic number, but more importantly, that's a HUGE epsilon value. Machine epsilon for a double type is about 1e-16. The amount of precision you need will vary, but when I did competitive programming I liked using const double EPS = 1e-9;.

It might be nice to be able to specify a precision when you're performing the equality check. To that end, it might be nice to have something like:

const double EPS = 1e-9;
bool similarValues(double a, double b, double eps = EPS) {
    return abs(a - b) < eps;
}

Answer 5

• Echoing others, your similarValues function should be returning a boolean. Calling it areSimilarValues reads better and is more descriptive.

• The two arguments are called smallerValue and biggerValue, yet you don't actually know that one is bigger than the other. It's equally as plausible you could call the function like this: similarValues(55, 0.1) in which case the variable names are completely wrong from their actual values. I would recommend something more generic in this case: a and b or value1 and value2.

• The similarValues function can be reduced to one line:

  #include <cmath>
  
  bool function areSimilarValues(double a, double b) {
      return abs(a - b) < 0.01;
  }
  

  Also note that no comments are required when you give things descriptive names, and use appropriate return types.

• Like others have said, the valueLarger and valueSmaller functions could use better names, such as getLargerValue and getSmallerValue.

• The valueLarger and valueSmaller functions return zero when the values are equal? The other return statements return one of the numbers. Returning zero feels like a logic error. If they are equal, just return one of them. It doesn't really matter which one you return since they are equal.

  double getLargerValue(double a, double b) {
      if (a > b) {
          return a;
      }
      else if (b > a) {
          return b;
      }
      else {
          // Values are equal
          return a;
      }
  }
  

  Even this could be reduced to:

  double getLargerValue(double a, double b) {
      return b > a ? b : a;
  }
  

  If b is greater than a, return b. Otherwise, a is either larger than b or a and b are equal, so return a. And just for good measure, the getSmallerValue function:

  double getSmallerValue(double a, double b) {
      return b < a ? b : a;
  }
  

• Now that I think about it, Mathematics has a term for getting the larger or smaller value of two numbers: Maximum and Minimum. These would be good names for valueLarger and valueSmaller:

  double maximum(double a, double b) {
  }
  
  double minimum(double a, double b) {
  }