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?

• MORTAL: you can use std::max and std::min to get larger and smaller values
• Loki Astari: I think you are doing fine. Small function of less than a dozen lines are perfect. Rule of Thumb: a function should not be larger than a terminal window so you can read the whole thing without scrolling (rule of thumb two: Don't make your font smaller so it will fit :-). Though you could do with practice naming the functions (and finding their equivalent in the standard). If you test for equality first then your other function become easier to write.
• nukeforum: @LokiAstari Another rule of thumb I use: if you have to rewrite the same block of code three times, make it a method... Although, perhaps that one doesn't apply to this particular program. :)

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.

• user91060: using namespace std is perfectly fine. It's bad if used on header files.
• Deduplicator: @Pr0kram3r: Did you read the post I linked? I concede that in a header-file it's a more obviously unconscionable evil, but being more subtle in implementation-files doesn't make it right.
• user91060: In a header file using "using" is a mistake. In a .cpp file I think you are on your own. If you don't have any name clashes I don't see any serious problem with a "using" clause.
• nukeforum: @Pr0kram3r using isn't the issue here. using namespace @ is. using std::string is fine.
• Ruslan: In fact, not even using namespace @ is a problem. The problem is with using such a huge and containing so many common words as identifiers (string, vector, pair, tuple, ends) namespace as std — even if not all the headers introducing these are included, they are allowed by the Standard to be included by each other.

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";
}

• nukeforum: @Ruslan cin.fail() should return true if the assignment fails due to a character/string being typed. Is this incorrect?
• Ruslan: Yes, it should fail, but first the operator>>() will return object convertible to false, and you won't enter the body of while.
• nukeforum: @Ruslan I see, so there is no checking to do. I will correct this.
• Ruslan: Have you tried to compile your final code, BTW? It doesn't compile because you've not #included <cmath>.
• nukeforum: @Ruslan Which compiler are you using?

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.

• Deduplicator: Not using std::endl is one of the points OP did right. You don't want to flush your performance down the drain without good reason. Though for the few cases he outputs a length-1-string, he should prefer outputting a single char.

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;
}

• nukeforum: I intended to address the magic number problem in my answer, but apparently I forgot to in the sea of everything else. Fantastic answer. +1
• Monty Harder: [Never mind. Looks like you've covered it]

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) {
  }
  

• Deduplicator: ad 3: The reduction is wrong if passing the not-smaller value first is no longer part of the contract. ad4: valueLarger and valueSmaller are more complicated and confusing beasts, as you later say. Your redefinition in point 5 should be quite reasonable. But there are standard-functions or that different concept...
• Greg Burghardt: @Deduplicator: What do you mean by "The reduction is wrong if passing the not-smaller value first is no longer part of the contract." for bullet point #3. The logic between the OP's code and my version appear to be the same.
• nukeforum: @GregBurghardt I think Deduplicator is trying to say that in your third bullet point, you don't account for a being smaller than b. In which case, the variance could still be .01, but the return would not register true because the value would be -.01. To fix this, just use abs() in the cmath lib.
• nukeforum: @GregBurghardt Speaking of cmath, the cmath lib also has fmax() and fmin(). No need to reinvent the wheel here.
• gnasher729: If I saw the functions valueLarger and valueSmaller in actual code, I would rename them to "weirdMaximum" and "weirdMinimum" to indicate they return something related to the minimum or maximum, but behave weird in some way.