Skip to main content
Code Review

Return to Answer

Take comment about <ctype.h>/<cctype> into account.
Source Link
Morwenn
Morwenn
  • 20k
  • 3
  • 67
  • 127

There are several small things that you can improve:

  • double weight = 0; works fine, but double weight = 0.0; would be more pedantic since weight is a double.

  • By the way, the following line is a good example to illustrate the benefits of literals pedantry:

      double volume = ((4/3) * M_PI * pow(obj.radius, 3));

    Here, 4/3 performs an integer division and not a floating point one; that expression will yield 1 instead of 1.333333333 (I doubt that you want it to yield 1). You should change it to 4.0/3.0 to get the desired result.

  • That said, there is another problem with the aforementioned line: M_PI is not standard C++. It is not standard C either. It is a standard POSIX addition to <math.h>. You should rewrite your own constants or use Boost ones for examples if you want your code to be portable.

  • Speaking of <math.h>... You are using C++, therefore you should use the C++ standard library headers too and not the C ones:

      #include <cmath>

  • And you notably forgot to include the header <cctype><ctype.h> for std::tolower.

  • AlsoThat said, it is good practice in C++ to fully qualify the names of the functions from the standard library: you should write std::tolower (and thus actually include <cctype> instead of <ctype.h>). It's not really longer and it may help to prevent name clashes (I doubt that you will have problems with tolower, but it can be worse with some more common names).

There are several small things that you can improve:

  • double weight = 0; works fine, but double weight = 0.0; would be more pedantic since weight is a double.

  • By the way, the following line is a good example to illustrate the benefits of literals pedantry:

      double volume = ((4/3) * M_PI * pow(obj.radius, 3));

    Here, 4/3 performs an integer division and not a floating point one; that expression will yield 1 instead of 1.333333333 (I doubt that you want it to yield 1). You should change it to 4.0/3.0 to get the desired result.

  • That said, there is another problem with the aforementioned line: M_PI is not standard C++. It is not standard C either. It is a standard POSIX addition to <math.h>. You should rewrite your own constants or use Boost ones for examples if you want your code to be portable.

  • Speaking of <math.h>... You are using C++, therefore you should use the C++ standard library headers too and not the C ones:

      #include <cmath>

  • And you notably forgot to include the header <cctype> for std::tolower.

  • Also, it is good practice in C++ to fully qualify the names of the functions from the standard library: you should write std::tolower. It's not really longer and it may help to prevent name clashes (I doubt that you will have problems with tolower, but it can be worse with some more common names).

There are several small things that you can improve:

  • double weight = 0; works fine, but double weight = 0.0; would be more pedantic since weight is a double.

  • By the way, the following line is a good example to illustrate the benefits of literals pedantry:

      double volume = ((4/3) * M_PI * pow(obj.radius, 3));

    Here, 4/3 performs an integer division and not a floating point one; that expression will yield 1 instead of 1.333333333 (I doubt that you want it to yield 1). You should change it to 4.0/3.0 to get the desired result.

  • That said, there is another problem with the aforementioned line: M_PI is not standard C++. It is not standard C either. It is a standard POSIX addition to <math.h>. You should rewrite your own constants or use Boost ones for examples if you want your code to be portable.

  • Speaking of <math.h>... You are using C++, therefore you should use the C++ standard library headers too and not the C ones:

      #include <cmath>

  • And you notably forgot to include the header <ctype.h> for std::tolower.

  • That said, it is good practice in C++ to fully qualify the names of the functions from the standard library: you should write std::tolower (and thus actually include <cctype> instead of <ctype.h>). It's not really longer and it may help to prevent name clashes (I doubt that you will have problems with tolower, but it can be worse with some more common names).

Slightly reformulated.
Source Link
Morwenn
Morwenn
  • 20k
  • 3
  • 67
  • 127

There are several small things that you couldcan improve:

  • double weight = 0; works fine, but double weight = 0.0; would be more pedantic since weight is a double.

  • You are using C++, therefore you should useBy the C++ standard library headers too and notway, the C ones:

      #include <cmath>

  • M_PI is not standard C++. It is not standard C either. Itfollowing line is a standard POSIX addition to <math.h>. You should rewrite your own constants or use Boost ones for examples if you want your codegood example to be portable.

  • There is probably an error in this pieceillustrate the benefits of codeliterals pedantry:

      double volume = ((4/3) * M_PI * pow(obj.radius, 3));

    Here, 4/3 performs an integer division and not a floating point division; itone; that expression will yield 1 instead of 1.333333333 (I doubt that you want it to yield 1). You should change it to 4.0/3.0 to get the desired result.

  • You usedThat said, there is another problem with the functionaforementioned line: tolowerM_PI without includingis not standard C++. It is not standard C either. It is a standard POSIX addition to <math.h>. You should rewrite your own constants or use Boost ones for examples if you want your code to be portable.

  • Speaking of <math.h>... You are using C++, therefore you should use the C++ standard library headers too and not the C ones:

      #include <cmath>

  • And you notably forgot to include the header <cctype> firstfor std::tolower. 

  • Also, it is good practice in C++ to fully qualify the names of the functions from the standard library: you should write std::tolower. It's not really longer and it may help to prevent name clashes (I doubt that you will have problems with tolower, but it can be worse with some more common names).

There are several small things that you could improve:

  • double weight = 0; works fine, but double weight = 0.0; would be more pedantic since weight is a double.

  • You are using C++, therefore you should use the C++ standard library headers too and not the C ones:

      #include <cmath>

  • M_PI is not standard C++. It is not standard C either. It is a standard POSIX addition to <math.h>. You should rewrite your own constants or use Boost ones for examples if you want your code to be portable.

  • There is probably an error in this piece of code:

      double volume = ((4/3) * M_PI * pow(obj.radius, 3));

    Here, 4/3 performs an integer division and not a floating point division; it will yield 1 instead of 1.333333333 (I doubt that you want it to yield 1). You should change it to 4.0/3.0 to get the desired result.

  • You used the function tolower without including <cctype> first. Also, it is good practice to fully qualify the names of the functions from the standard library: you should write std::tolower.

There are several small things that you can improve:

  • double weight = 0; works fine, but double weight = 0.0; would be more pedantic since weight is a double.

  • By the way, the following line is a good example to illustrate the benefits of literals pedantry:

      double volume = ((4/3) * M_PI * pow(obj.radius, 3));

    Here, 4/3 performs an integer division and not a floating point one; that expression will yield 1 instead of 1.333333333 (I doubt that you want it to yield 1). You should change it to 4.0/3.0 to get the desired result.

  • That said, there is another problem with the aforementioned line: M_PI is not standard C++. It is not standard C either. It is a standard POSIX addition to <math.h>. You should rewrite your own constants or use Boost ones for examples if you want your code to be portable.

  • Speaking of <math.h>... You are using C++, therefore you should use the C++ standard library headers too and not the C ones:

      #include <cmath>

  • And you notably forgot to include the header <cctype> for std::tolower. 

  • Also, it is good practice in C++ to fully qualify the names of the functions from the standard library: you should write std::tolower. It's not really longer and it may help to prevent name clashes (I doubt that you will have problems with tolower, but it can be worse with some more common names).

Source Link
Morwenn
Morwenn
  • 20k
  • 3
  • 67
  • 127

There are several small things that you could improve:

  • double weight = 0; works fine, but double weight = 0.0; would be more pedantic since weight is a double.

  • You are using C++, therefore you should use the C++ standard library headers too and not the C ones:

      #include <cmath>

  • M_PI is not standard C++. It is not standard C either. It is a standard POSIX addition to <math.h>. You should rewrite your own constants or use Boost ones for examples if you want your code to be portable.

  • There is probably an error in this piece of code:

      double volume = ((4/3) * M_PI * pow(obj.radius, 3));

    Here, 4/3 performs an integer division and not a floating point division; it will yield 1 instead of 1.333333333 (I doubt that you want it to yield 1). You should change it to 4.0/3.0 to get the desired result.

  • You used the function tolower without including <cctype> first. Also, it is good practice to fully qualify the names of the functions from the standard library: you should write std::tolower.