4
\$\begingroup\$
// ConsoleApplication1.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include <cstdlib>
#include <iostream>
#include <cmath>

using namespace std;

int main() {
    system("cls");
    system("color 5");

    float a;
    float b;
    float c;
    float Sector1Arithmetic;
    float Sector2Arithmetic;
    float Sector3Arithmetic;
    float Sector4Arithmetic;
    float Sector5Arithmetic;
    float X1;
    float X2;

    cout << endl;
    cout << "            Quadratic Equation Solver";
    cout << endl;
    cout << endl;
    cout << "       Enter the value of the a co-efficient ---> ";
    cin >> a;
    cout << endl;
    cout << endl;
    cout << "       Enter the value of the b co-efficient ---> ";
    cin >> b;
    cout << endl;
    cout << endl;
    cout << "       Enter the value of the c co-efficient ---> ";
    cin >> c;
    cout << endl;

    Sector1Arithmetic = b * -1;
    Sector2Arithmetic = b * b - 4 * a * c;
    Sector3Arithmetic = sqrt(Sector2Arithmetic);
    Sector4Arithmetic = Sector1Arithmetic + Sector3Arithmetic;
    Sector5Arithmetic = Sector1Arithmetic - Sector3Arithmetic;
    X1 = Sector4Arithmetic / 2 * a;
    X2 = Sector5Arithmetic / 2 * a;

    cout << "  For the equation " << a << "x^2 + " << b << "x + " << c <<     endl;
    cout << endl;
    cout << "  X=" << X1 << " OR " << "X=" << X2 << endl;

    if ((-4 * a * c) >= (b * b) || (a == 0) || (b == 0) || (c == 0)) {
    cout << endl;
    cout << "An error might have occurred due to impractical math calculations" << endl;
    cout << "OR due to invalid values inputted such as 0" << endl;
    cout << endl;
    }

    cout << endl;
    system("pause");
    main();
    return 0;

}

As you may remember I also posted a quadratic equation solver in Python as well. The main reason I post this simple program is to get critical feedback and perhaps look for alternative ways of coding such simple program applying it to my future projects.

\$\endgroup\$
5
\$\begingroup\$

A couple other issues I noted that haven't been mentioned.

  • You're inconsistent with capitalization of the x variable. The text output lists it as 'x' in the equation and 'X' in the solutions. Also, it's the only variable you declare as a capital (except the SectorNArithmetic variables). I would definitely print lowercase as that's typically used in math. I typically use lowercase for single-letter C variables but that's more of a preference thing -- just be consistent.
  • You change the color of the screen when you start the program, but never change it back. Now I have to close my console or manually change colors if I want my colors back.
  • There's no way to quit the program without Ctrl+C or similar.
  • You don't check input for validity.

Some more trivial issues:

  • The equation is missing the "0 = " part of "0 = ax^2 + bx +c" in the output.
  • There's no need for all the leading whitespace. If you want to center the first title, that makes sense, but the rest is just wasting screen real estate.
  • If b or c are negative, you print plus and minus, like "+ -4.5". It would look prettier if you printed "- 4.5" in this case.
  • You don't handle imaginary numbers. This isn't terribly difficult to do -- if the discriminant is negative, handle the real and imaginary parts separately, otherwise do things normally.
  • You don't properly handle cases where a is 0. This is just \$0=0x^2+bx+c\$ \$=bx+c\$ \$\leftrightarrow x={-c\over b}\$.
  • If there's a single root (the discriminant is 0), you print it twice.

Here's some code that fixes these issues. It's not the best way to do everything, but should give you some ideas.

// QuadEqnApp.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include <cstdlib>
#include <iostream>
#include <cmath>
#include <Windows.h>

using namespace std;

int main() {

    /* Variable declaration for the main program. */
    float a;    // Coefficient a.
    float b;    // Coefficient b.
    float c;    // Coefficient c.
    float d;    // Discriminant.
    float sq;   // Square root of the discriminant.
    bool i;     // sq is imaginary?
    float q;    // Quotient of the root.
    float xr;   // The real part of both roots.
    float xi;   // The potentially imaginary part of both roots.
    char test;  // Input to test for try again.
    bool again; // Are we trying again?

    /* Setup the colors for this program. */
    CONSOLE_SCREEN_BUFFER_INFO csbi;                    // The user's current font settings.
    HANDLE hstdout = GetStdHandle(STD_OUTPUT_HANDLE);   // Get a handle to stdout.
    GetConsoleScreenBufferInfo(hstdout, &csbi);         // Get all the current font info.
    SetConsoleTextAttribute(hstdout, 0x5);              // Set the color for this program.

    // Main program loop. Repeats until the user asks to quit.
    do
    {
        /* Get all the info from the user. */
        system("cls");  // Clear the screen each time.
        cout << endl;
        cout << "       Quadratic Equation Solver";
        cout << endl << endl;
        cout << "Enter the value of the a co-efficient ---> ";
        while (!(cin >> a)) {   // Make sure the user actually enters a float.
            cin.clear();
            cin.ignore(99999, '\n');
            cout << "You did not enter a number, please try again: ";
        }
        cin.clear();    // Clear the input in case the user entered something like "222.3sdkdk" -- 222.3 will be in a, but the sdkdk will be hanging out.
        cin.ignore(99999, '\n');
        cout << endl << endl;
        cout << "Enter the value of the b co-efficient ---> ";
        while (!(cin >> b)) {   // Make sure the user actually enters a float.
            cin.clear();
            cin.ignore(99999, '\n');
            cout << "You did not enter a number, please try again: ";
        }
        cin.clear();
        cin.ignore(99999, '\n');
        cout << endl << endl;
        cout << "Enter the value of the c co-efficient ---> ";
        while (!(cin >> c)) {   // Make sure the user actually enters a float.
            cin.clear();
            cin.ignore(99999, '\n');
            cout << "You did not enter a number, please try again: ";
        }
        cin.clear();
        cin.ignore(99999, '\n');
        cout << endl;

        /* Pretty print the equation line. */
        cout << "For the equation 0 = " << a << "x^2 ";
        cout << ((b < 0) ? "- " : "+ ") << ((b < 0) ? -b : b);  // Ternary operators to print "+ bx" or "- bx", not "+ -bx".
        cout << "x ";
        cout << ((c < 0) ? "- " : "+ ") << ((c < 0) ? -c : c);  // Ternary operators to print "+ c" or "- c", not "+ -c".
        cout << endl << endl;

        /* Pretty print the roots. */
        if (a != 0) {   // If a isn't 0, use the quadratic equation.

            /* Do most of the math stuff. */
            d = b * b - 4 * a * c;      // Get the discriminant.
            i = (d < 0);                // Determine whether the root is real or imaginary.
            if (i) d = -d;              // Make sure d is positive.
            sq = sqrt(d);               // Get the square root of the magnitude of the discriminant.
            q = 2 * a;                  // Get the quotient.
            xr = (b != 0) ? -b / q : 0; // Get the always real part of the root. Ternary operator to prevent printing -0 if b is 0.
            xi = sq / q;                // Get the possibly imaginary part of the root.
            if (xi < 0) xi = -xi;       // Make sure xi is positive so it will print in descending order.

            /* Print the correct format depending on how many roots (1 or 2) and if it's real or imaginary. */
            if (!i) {   // If the discriminant is positive, print the real format.
                if (xi != 0) {  // If the discriminant isn't zero, print two roots.
                    cout << "x = " << (xr + xi) << " OR x = " << (xr - xi) << endl;
                }
                else {          // If the discriminant is zero, there's just one root.
                    cout << "x = " << xr << endl;
                }
            }
            else {      // If the discriminant is negative, print the imaginary format.
                if (xr != 0) {  // If the real part isn't zero, we need to print it.
                    cout << "x = " << xr << " + " << xi << "i OR x = " << xr << " - " << xi << "i" << endl;
                }
                else {          // If the real part is zero, don't print it.
                    cout << "x = " << xi << "i OR x = " << (-xi) << "i" << endl;
                }
            }
        }
        else {  // If a is 0, use the simpler equation, x = -c/b.
            if (b != 0) {   // If b isn't zero, just do the math.
                cout << "x = " << (-c / b) << endl;
            }
            else {          // If b is zero, there is no root.
                cout << "There is no root." << endl;
                if (c == 0) {   // At least the equation was valid.
                    cout << "But the equation evaluates to 0 = 0, which is true." << endl;
                }
                else {          // The equation wasn't even valid.
                    cout << "And the equation evaluates to 0 = " << c << ", which is false." << endl;
                }
            }
        }

        /* Decide whether to run the main loop again. */
        cout << endl << "Try again? Y/N" << endl;
        cin >> test;
        again = (test == 'Y' || test == 'y');
    } while (again);

    /* Set the screen back to its original settings and quit. */
    SetConsoleTextAttribute(hstdout, csbi.wAttributes);
    system("cls");
    return 0;
}
  • I didn't use any OOP. Creating a class with members is one of the main reasons to use C++ over C. Michael Anderson's answer shows this pretty well.
  • I'm not actually sure that I've ever done a real C++ program. Mostly I do C for microcontrollers or something like Java or C# for PCs. So take my comments with a dose of salt.
  • I don't know if there's a better way to save color data and set it back. I got that code from here, and he recommends not using system() at all, so it's probably weird to mix that code with the system("cls") calls.
  • To be realistic, there's probably zero reason to change the color here. If I set my console to green text, it's because I like green text. When you change it just because you like purple text, it doesn't make me happy. The only good reason to force color changes is if you want to highlight specific words, phrases or symbols. Or if you're making a game using ASCII text and the text is essentially graphical data.
  • I use a lot of comments. If you use more verbose variables names, a lot of the comments can be removed since "Discriminant = ..." is obviously getting the discriminant. On the other hand, doing long math equations with verbose names can be a lot harder to read.
  • You may not need/want all the functionality I've added.
  • I shortened a few lines by using ternary operators instead of if{}else{} constructs. You could always use the more verbose constructs, but I find spending 10 lines doing something simple can be more confusing than having to spend a little more time reading one line.
  • Similarly, I did a few simple one-line if statements for the same reasons. Both of these are preference, and you can do it how you want.
  • I got the input-checking code from this answer and modified it a bit. It seems to work, but I can't promise it's perfect.
  • I didn't create separate methods for the various sections of the code. Personally, I wouldn't bother for something this trivial, but as the code gets longer, it can help to get the code out of main() and into secondary methods. The three inputs would be an obvious place for code re-use.
  • On the other hand, storing 2*a into q to use it twice is probably not necessary (I originally used q like 6 times which is why I did it that way).
  • You can save memory by re-using variables. Instead of d = b*b-4*a*c then sq = sqrt(d), you could just do something like sqd=b*b-4*a*c;...;sqd=sqrt(sqd);. It doesn't really matter here, but it comes down to whether you want more readable code with individual steps, or better performance. Breaking it into too many steps can be less clear, but since I had to test the discriminant before taking its square root I just saved it to separate variables.
  • Declaration of variables d through xi could be done inside the if branch that uses them instead of up front. This would reduce the scope (less chance of naming conflicts), reduce the amount of memory used on average, and possibly be more readable (local declarations right where they're used instead of scrolling way up to find them). However, it also means you're re-declaring the variables every time you run the loop again, which could be slightly slower if you're doing an automated loop instead of waiting for user input. And since those variables are going to be used like 99% of the time, they aren't that out of scope.
  • I used block comments (/* */) as headers. This is probably a bad idea, but several C/C++ classes I've taken do it this way. It makes it hard to do /* */ { code here } /* */ then just delete the second '/' to quickly comment/uncomment entire blocks of code. On the other hand, that may not be the best way to do things either.
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Very thorough first answer! One note, if you're showing code in your review it's often more helpful to break up the code as you go, rather than having one long block of code. Unless you find that the only way to demonstrate all your changes is together, rather than with separate blocks. \$\endgroup\$ – SuperBiasedMan Oct 20 '15 at 10:55
11
\$\begingroup\$

You have committed a grave error here with operator precedence, which should have easily been caught with simple testing:

X1 = Sector4Arithmetic / 2 * a;

The variable names SectornArithmetic are bizarre. Most of those intermediate results don't need to be assigned to a variable. The most complex intermediate result, b * b - 4 * a * c, has a standard name: "discriminant", which you should use.

You can use if (discriminant < 0) to detect that there are no real roots. The other tests — (b == 0) and (c == 0) — are superfluous and harmful, as there is no justification for prohibiting those coefficients from being 0.

As mentioned in the previous Python review, system("pause") is inappropriate. Just call std::getline() and discard the result.

Calling main() recursively is worse than a goto, as you are deepening the call stack for no reason. Write a proper loop instead. Furthermore, the return 0; statement is unreachable and should be eliminated.

\$\endgroup\$
  • 6
    \$\begingroup\$ Note: In C++, using main at all is an error: stackoverflow.com/questions/4518598/… \$\endgroup\$ – Deduplicator Oct 19 '15 at 19:38
  • \$\begingroup\$ I'd probably recommend not using system( ... ); at all if it's use is superfluous, which is most likely the case in this code. \$\endgroup\$ – Ethan Bierlein Oct 20 '15 at 11:20
7
\$\begingroup\$
  1. Separate IO and logic.
  2. Group logical parts into classes / functions.
  3. Declare variables where they are used.
  4. Declare IO routines for classes when appropriate.

I'd also seriously reconsider using calls to system and recursion into main() - but I'm not going to go into that here.

Applying these you might get something like:

class QuadraticEquation {
  public:
  float a;
  float b;
  float c;

  float discriminant() const {
    return b * b - 4 * a * c;
  }

  boolean hasRealRoots() const {
    return discriminant()>=0;
  }

  float getRoot1() const {
     return (-b + sqrt(discriminant())) / (2*a);
  }

  float getRoot2() const {
     return (-b - sqrt(discriminant())) / (2*a);
  }
};

QuadraticEquation getEquationFromUser() {
    QuadraricEquation eq;
    cout << endl;
    cout << "            Quadratic Equation Solver";
    cout << endl;
    cout << endl;
    cout << "       Enter the value of the a co-efficient ---> ";
    cin >> eq.a;
    cout << endl;
    cout << endl;
    cout << "       Enter the value of the b co-efficient ---> ";
    cin >> eq.b;
    cout << endl;
    cout << endl;
    cout << "       Enter the value of the c co-efficient ---> ";
    cin >> eq.c;
    cout << endl;
    return eq;
}

std::ostream& operator<<(std::ostream & os, const QuadraticEquation & eq ) {
   return os<< a << "x^2 + " << b << "x + " << c;
}

int main() {
    system("cls");
    system("color 5");

    QuadratricEquation eq = getEquationFromUser();

    cout << "  For the equation " << eq << endl;
    cout << endl;
    if(eq.hasRealRoots()) {
       cout << "  X=" << eq.getRoot1() << " OR " << "X=" << eq.getRoot2() << endl;
    } else {
       cout<<" there are no real roots"<<endl;
    }

    cout << endl;
    system("pause");
    main();
    return 0;

}
\$\endgroup\$
  • \$\begingroup\$ Very nice breakdown! \$\endgroup\$ – user1118321 Oct 20 '15 at 3:51
  • \$\begingroup\$ Note that if a and b have opposite signs, the "largest" root can be smaller than the "smallest" root. For example, if \$a=-1, b=10, c=1\$, you have \$-b-\sqrt{d}\over 2a\$\$\approx 10.1\$ and \$-b+\sqrt{d}\over 2a\$\$\approx -0.1\$. Personally, I would call them getPlusRoot and getMinusRoot or just getRoot1 and getRoot2. \$\endgroup\$ – MichaelS Oct 20 '15 at 6:45
  • \$\begingroup\$ @MichaelS - agreed. It will also explode when a=0. \$\endgroup\$ – Michael Anderson Oct 20 '15 at 7:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.