patterncppMinor
Square root approximation with Newton's method
Viewed 0 times
approximationmethodwithnewtonrootsquare
Problem
I designed a program that calculates the square root of a number using Newton's method of approximation that consists of taking a guess (
Can you see any way to improve this ? Like in the "do it again" part, I just couldn't use y/n...
g) and improving it (improved_guess = (x/g + g)/2) until you can't improve it anymore:#include
#include
using namespace std;
template
Y sqrt (Y x)
{
double g (1), ng;
while (true) {
ng = (x/g + g)/2;
if (g != ng) g = ng;
else if (g == ng) break;
}
return g;
}
void menu()
{
double x, g;
string a = "";
do {
cout > x;
g = sqrt(x);
cout ";
cin >> a;
cout << endl;
} while (a == "y");
}
int main()
{
menu();
return 0;
}Can you see any way to improve this ? Like in the "do it again" part, I just couldn't use y/n...
Solution
Looks fairly good over all, but a few things jumped out at me. Please note that the stylistic ones are opinion based.
Make sure you don't get into a habit of
If you're not going to put your function in a namespace, I would avoid calling it
I would leave a blank space after your header lines. The typical format is:
Also, I would consider alphabetizing the headers. When there's a long list of headers, it can help make quick mental scanning easier.
You forgot to include
Your
I don't like the use of the constructor form initialization of primitives. I would stick with plain old equals.
Also, it's fairly typical to define one variable per line when values are being assigned during declaration.
I'm not quite sure why
Consider taking
In menu, you should check if reading in a double was successful:
Try to declare variables as close to usage as possible. For example, your
If you're using C++11, avoiding naming types except where necessary. This will ease future change of type. For example, imagine that at some point you want to use some kind of Complex class instead of a built in double. If you only name the type for the input
When it's not possible for the program to return a non-0 exit code, I like to omit it. This signifies at a simple glance that the program always exits with a success code.
Make sure you don't get into a habit of
using namespace std;. It's acceptable in some places, but it can form a bad habit very easily. Your code is a perfect example of this since your sqrt will conflict with std::sqrt if you include cmath or math.h. A lot more discussion on the matter can be found here.If you're not going to put your function in a namespace, I would avoid calling it
sqrt. In fact, unless you have a compelling reason, it's best to avoid names from the standard library in general.I would leave a blank space after your header lines. The typical format is:
#include "local1.h"
#include "local2.h"
#include
#include
// stuff hereAlso, I would consider alphabetizing the headers. When there's a long list of headers, it can help make quick mental scanning easier.
You forgot to include
string.Your
c variable in sqrt isn't used. I don't like the use of the constructor form initialization of primitives. I would stick with plain old equals.
Also, it's fairly typical to define one variable per line when values are being assigned during declaration.
double g = 1;
double ng;I'm not quite sure why
sqrt is templated. By using doubles internally, you've basically restricted it to doubles. If you're going to make it templated, use the templated type all the way through.Y is a bad typename. Use something more descriptive like FloatingPoint or Number.Consider taking
x by const reference in sqrt. If an expensive-to-copy class is used as the template parameter, you're causing an unnecessary copy since x is never modified.In menu, you should check if reading in a double was successful:
if (std::cin >> x) {
// Use x
} else {
std::cerr << "Invalid value provided\n";
}Try to declare variables as close to usage as possible. For example, your
g = sqrt(x); could be double g = sqrt(x);. Seeing a giant list of variables declared at the top of a function is overwhelming. If they're defined where they're used, it's much simpler to see their relation to the overall code.If you're using C++11, avoiding naming types except where necessary. This will ease future change of type. For example, imagine that at some point you want to use some kind of Complex class instead of a built in double. If you only name the type for the input
x and use auto else where, you'll only have to change one thing (provided the class overrode operator>> for istreams). You can read more about this here.When it's not possible for the program to return a non-0 exit code, I like to omit it. This signifies at a simple glance that the program always exits with a success code.
Code Snippets
#include "local1.h"
#include "local2.h"
#include <blah1>
#include <blah2>
// stuff heredouble g = 1;
double ng;if (std::cin >> x) {
// Use x
} else {
std::cerr << "Invalid value provided\n";
}Context
StackExchange Code Review Q#43456, answer score: 5
Revisions (0)
No revisions yet.