Please help, Can i improve this program :)

[Shashank]

Hello Team,

Can you guys suggest me the improvements for the below program :)

Thank you


//This program prints the roots of quadratic equation
#include"../../../std_lib_facilities.h"
class Bad_roots{};
void roots_Of_Quadratic_Equation(int a,int b,int c)
{
double root1,root2;
double squareRootValue=b*b-4*a*c;
if(squareRootValue<0)
throw Bad_roots();
else
{
root1=(-b+sqrt(squareRootValue))/2*a;
root2=(-b-sqrt(squareRootValue))/2*a;

cout<<"Roots of quadratic equation "<<a<<"x * x + "<<b<<" x +
"<<c<<"\t\t is "<<root1<<"\t"<<root2<<"\n";
}
}

int main()
try {
int a,b,c;
cout<<"Please enter the values of quadratic equation:\n";
cin>>a>>b>>c;
cout<<"You entered equation is "<<a<<"x * x + "<<b<<" x + "<<c<<"\n";
roots_Of_Quadratic_Equation(a,b,c);
keep_window_open();
return 0;

}
catch(Bad_roots)
{
cerr<<"Roots are imaginary:\n";
keep_window_open();
return 1;
}

[Art Werschulz]

Hi.

On May 22, 2012, at 8:52 AM, Shashank wrote:

> //This program prints the roots of quadratic equation
> #include"../../../std_lib_facilities.h"

General comment: use a bit of vertical space to enhance readability, e.g.,
#include "../../../std_lib_facilities.h"
Ditto with respect to non-multiplicative operators. I know that Bjarne doesn't do this, but itmakesformorereadablecode. :-)

> class Bad_roots{};
> void roots_Of_Quadratic_Equation(int a,int b,int c)

The function's name can be more concise. Moreover, why not allow the coefficients to have decimal points? So:
void solve_quadratic(double a, double b, double c)

> {
> double root1,root2;
> double squareRootValue=b*b-4*a*c;

Use the standard name for this quantity:
double discriminant = b*b - 4*a*c;

> if(squareRootValue<0)

You might get better roundoff error by using
if (b*b < 4*a*c)


> throw Bad_roots();
> else

You don't need to use if/else; an if-stmt will suffice. After all, the only way you can get here is if the quadratic has real roots.

> {
> root1=(-b+sqrt(squareRootValue))/2*a;
> root2=(-b-sqrt(squareRootValue))/2*a;

Why calculate the square root twice? Also "2*a" needs to be parenthesized.

So I'd start things off as follows:
if (b*b < 4*a*c)
throw Bad_roots();
double discr_sqrt = sqrt(b*b - 4*a*c);
double root1 = (-b + discr_sqrt)/(2*a);
double root2 = (-b - discr_sqrt)/(2*a);

Better yet: Distinguish between a multiple root of -b/(2*a) and two distinct real roots. You can save a bit of arithmetic (not very important) and you can give more informative output to the user (more important).

> cout<<"Roots of quadratic equation "<<a<<"x * x + "<<b<<" x +
> "<<c<<"\t\t is "<<root1<<"\t"<<root2<<"\n";

I'll assume that this is one very long line that got word-wrapped. If not, then the second line needs to be properly indented.

An easily-overlooked problem: If a is zero, then you have a division by zero when you calculate the two roots. In this case, the quadratic equation degenerates to a linear equation b*x + c = 0. How do you want to handle this? If you decide to solve the linear equation, there will be several cases to consider, which I'll let you think about.

Closing comment: solve_quadratic() does two things: it solves the equation and prints the answer. This means that you don't have the ability to do anything else with the computed solution. It would make more sense for solve_quadratic() to simply calculate the solution, giving it back in some form or another. The classical way to do this is via reference parameters (Chapter 8, IIRC). (There's another way to do this using structs, but it's a bit messy.)

Another closing comment: I wouldn't expect you to know this, but the classical quadratic formula is numerically unstable. Once you're convinced that your program is correct (by trying it on a bunch of quadratic equations whose roots you know), try it with
a = 1, b = 20000, c = 1.5e-9. One of the calculated roots will be zero, which has 100% relative error. This *can* be fixed, but that's a problem for another day.


Art Werschulz (8-{)} "Metaphors be with you." -- bumper sticker
GCS/M (GAT): d? -p+ c++ l u+(-) e--- m* s n+ h f g+ w+ t++ r- y?
Internet: agw STRUDEL comcast.net

[Vivekanand P V]

Dear Shashank,

My observations:
1. You need to use shorter yet intuitive names for your variables and
functions. Consult PPP style guide (It's in PPP support website). Or,
look at the way author names his variables and other constructs.
2. It is unwise to use a function to do more than what it is supposed
to do. That is, your root calculator function should only be concerned
with computing roots, while your code forces it to output the
result(s). This is inelegant. But how do you "return" two (or more)
values? You can either use a "struct" or a "class" (I'd rather go with
good old struct in this case). By the way, do you know how to use
one?

If you have not yet encountered it in the PPPUC++, one can presume
your code is all right to an extent.

Regards,
V

[Shashank]

thank you, for the inputs, I will implement all the suggestions in the
next exercise.

once gain thank you.