Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
  1. Your interface is missing a virtual destructor. This is an important thing for an abstract interface and your code is actually invoking undefined behavior when trying to destroy an instance of IRandom without one. With C++11, the proper way of declaring a virtual destructor in a pure interface class is using the = default destructor idion:

     virtual ~IRandom() = default;
    
  2. Using double_t is very unusual, since double is normally the same but with less typing. Unless you really have a good reason for double_t, you should stick to the native double instead. I would say the same about using int32_t. Unless you really care about using strictly sized 32bit integers, just go for a plain ol' int.

  3. By the way, when using the sized types, like int32_t and friends, from <cstdint>, make sure to include the header file. You are relying on a residual include probably from <random>, which might not be there on other compilers or even on a future version of your current one. At any rate, it is also good practice making dependencies clear.

  4. When using the types from <cstdint>, the correct way of referencing its members is thru the std:: namespace. They happen to also visible at the global level because on most compilers stdint.h (the C header) and cstdint (the C++ header) are implemented as wrappers to the same file. There is no requirement from the C++ Standard that this be implemented is such way, so a C++ compiler is not required to provide int32_t and friends outside the std namespace when including cstdint.

  5. std::numeric_limits<T>::[min][max]() is preferred over the INT*_MAX / INT*_MIN macros.

  6. Prefixing in-class member access with this-> is very rare in C++. You should only be doing that when, for instance, you need to disambiguate some shadowed member access (but even that is discouraged because shadowing members is a bit of a design flaw; better to uniquely name your variables and methods).

  7. Identifiers beginning with underscore carry a few restrictions in C++ (Read this Read this). Your use inside a class for member variables is not wrong, but might be frowned upon. Consider perhaps other styles, such as the m_prefix or and underscore atTheEnd_. Or no distinction at all for member variables.

  8. I think you probably meant to camelCase _bytedistribution but mistyped the d in "distribution".

  9. This is not the ideal way of initializing data in a constructor:

  1. Your interface is missing a virtual destructor. This is an important thing for an abstract interface and your code is actually invoking undefined behavior when trying to destroy an instance of IRandom without one. With C++11, the proper way of declaring a virtual destructor in a pure interface class is using the = default destructor idion:

     virtual ~IRandom() = default;
    
  2. Using double_t is very unusual, since double is normally the same but with less typing. Unless you really have a good reason for double_t, you should stick to the native double instead. I would say the same about using int32_t. Unless you really care about using strictly sized 32bit integers, just go for a plain ol' int.

  3. By the way, when using the sized types, like int32_t and friends, from <cstdint>, make sure to include the header file. You are relying on a residual include probably from <random>, which might not be there on other compilers or even on a future version of your current one. At any rate, it is also good practice making dependencies clear.

  4. When using the types from <cstdint>, the correct way of referencing its members is thru the std:: namespace. They happen to also visible at the global level because on most compilers stdint.h (the C header) and cstdint (the C++ header) are implemented as wrappers to the same file. There is no requirement from the C++ Standard that this be implemented is such way, so a C++ compiler is not required to provide int32_t and friends outside the std namespace when including cstdint.

  5. std::numeric_limits<T>::[min][max]() is preferred over the INT*_MAX / INT*_MIN macros.

  6. Prefixing in-class member access with this-> is very rare in C++. You should only be doing that when, for instance, you need to disambiguate some shadowed member access (but even that is discouraged because shadowing members is a bit of a design flaw; better to uniquely name your variables and methods).

  7. Identifiers beginning with underscore carry a few restrictions in C++ (Read this). Your use inside a class for member variables is not wrong, but might be frowned upon. Consider perhaps other styles, such as the m_prefix or and underscore atTheEnd_. Or no distinction at all for member variables.

  8. I think you probably meant to camelCase _bytedistribution but mistyped the d in "distribution".

  9. This is not the ideal way of initializing data in a constructor:

  1. Your interface is missing a virtual destructor. This is an important thing for an abstract interface and your code is actually invoking undefined behavior when trying to destroy an instance of IRandom without one. With C++11, the proper way of declaring a virtual destructor in a pure interface class is using the = default destructor idion:

     virtual ~IRandom() = default;
    
  2. Using double_t is very unusual, since double is normally the same but with less typing. Unless you really have a good reason for double_t, you should stick to the native double instead. I would say the same about using int32_t. Unless you really care about using strictly sized 32bit integers, just go for a plain ol' int.

  3. By the way, when using the sized types, like int32_t and friends, from <cstdint>, make sure to include the header file. You are relying on a residual include probably from <random>, which might not be there on other compilers or even on a future version of your current one. At any rate, it is also good practice making dependencies clear.

  4. When using the types from <cstdint>, the correct way of referencing its members is thru the std:: namespace. They happen to also visible at the global level because on most compilers stdint.h (the C header) and cstdint (the C++ header) are implemented as wrappers to the same file. There is no requirement from the C++ Standard that this be implemented is such way, so a C++ compiler is not required to provide int32_t and friends outside the std namespace when including cstdint.

  5. std::numeric_limits<T>::[min][max]() is preferred over the INT*_MAX / INT*_MIN macros.

  6. Prefixing in-class member access with this-> is very rare in C++. You should only be doing that when, for instance, you need to disambiguate some shadowed member access (but even that is discouraged because shadowing members is a bit of a design flaw; better to uniquely name your variables and methods).

  7. Identifiers beginning with underscore carry a few restrictions in C++ (Read this). Your use inside a class for member variables is not wrong, but might be frowned upon. Consider perhaps other styles, such as the m_prefix or and underscore atTheEnd_. Or no distinction at all for member variables.

  8. I think you probably meant to camelCase _bytedistribution but mistyped the d in "distribution".

  9. This is not the ideal way of initializing data in a constructor:

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

Note: Herb Sutter seems to disagree (thanks @ChrisDrew @ChrisDrew for the comment). Personally, I am down for auto and type inference whenever it makes code shorter or easier to read. In this particular case, it is just more typing (which defeats one of the purposes of type inference, BTW). The constructor initialization syntax has been around since day 1, so it is pretty well rooted and known. The main argument for the auto var = Type() style would be to make things more uniform, however, when declaring a member variable, the type still has to go on the left hand side, so the argument for consistency is only partial.

Note: Herb Sutter seems to disagree (thanks @ChrisDrew for the comment). Personally, I am down for auto and type inference whenever it makes code shorter or easier to read. In this particular case, it is just more typing (which defeats one of the purposes of type inference, BTW). The constructor initialization syntax has been around since day 1, so it is pretty well rooted and known. The main argument for the auto var = Type() style would be to make things more uniform, however, when declaring a member variable, the type still has to go on the left hand side, so the argument for consistency is only partial.

Note: Herb Sutter seems to disagree (thanks @ChrisDrew for the comment). Personally, I am down for auto and type inference whenever it makes code shorter or easier to read. In this particular case, it is just more typing (which defeats one of the purposes of type inference, BTW). The constructor initialization syntax has been around since day 1, so it is pretty well rooted and known. The main argument for the auto var = Type() style would be to make things more uniform, however, when declaring a member variable, the type still has to go on the left hand side, so the argument for consistency is only partial.

Made an addendum.
Source Link
glampert
  • 17.3k
  • 4
  • 31
  • 89

Note: Herb Sutter seems to disagree (thanks @ChrisDrew for the comment). Personally, I am down for auto and type inference whenever it makes code shorter or easier to read. In this particular case, it is just more typing (which defeats one of the purposes of type inference, BTW). The constructor initialization syntax has been around since day 1, so it is pretty well rooted and known. The main argument for the auto var = Type() style would be to make things more uniform, however, when declaring a member variable, the type still has to go on the left hand side, so the argument for consistency is only partial.

Note: Herb Sutter seems to disagree (thanks @ChrisDrew for the comment). Personally, I am down for auto and type inference whenever it makes code shorter or easier to read. In this particular case, it is just more typing (which defeats one of the purposes of type inference, BTW). The constructor initialization syntax has been around since day 1, so it is pretty well rooted and known. The main argument for the auto var = Type() style would be to make things more uniform, however, when declaring a member variable, the type still has to go on the left hand side, so the argument for consistency is only partial.

added 967 characters in body
Source Link
glampert
  • 17.3k
  • 4
  • 31
  • 89
Loading
Added an extra point; Made other minor adjustments.
Source Link
glampert
  • 17.3k
  • 4
  • 31
  • 89
Loading
Source Link
glampert
  • 17.3k
  • 4
  • 31
  • 89
Loading
lang-cpp

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