Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

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 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 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 <blah1>
#include <blah2>
// stuff here

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 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.

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 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 <blah1>
#include <blah2>
// stuff here

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 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.

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 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 <blah1>
#include <blah2>
// stuff here

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 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.

added 44 characters in body
Source Link
Corbin
  • 10.6k
  • 2
  • 30
  • 51

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 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::sqrtstd::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 <blah1>
#include <blah2>
// stuff here

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 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 need toshould 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.

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 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 <blah1>
#include <blah2>
// stuff here

Also, I would consider alphabetizing the headers. When there's a long list of headers, it can help make quick mental scanning easier.


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 need to 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);.


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).


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.

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 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 <blah1>
#include <blah2>
// stuff here

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 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.

Source Link
Corbin
  • 10.6k
  • 2
  • 30
  • 51

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 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 <blah1>
#include <blah2>
// stuff here

Also, I would consider alphabetizing the headers. When there's a long list of headers, it can help make quick mental scanning easier.


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 need to 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);.


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).


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.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /