Skip to main content
added 710 characters in body
Source Link
Jamal
  • 34.9k
  • 13
  • 133
  • 237
  • It's usually preferred to have your own headers before library headers. This can help avoid possible dependencies and also keep the headers more organized.

  • Do not use using namespace std in a header file. It can be okay to have it in an implementation file due to the locality (.cpp files do not get imported), but having it in a header file will force users to have the std namespace exposed, which can possibly break their code. Just leave it out and use std:: where appropriate.

  • Macro constants (or macros in general) are not very common in C++, and are primarily discouraged as there are better ways of doing things. Specifically, the const keyword should be used to make something constant and prevent any further changes to it.

      const int WORLD_HEIGHT = 4;
    

    Moreover, as these are in a header file, they will be global by default. While it is not such a bad thing since they cannot be modified, it would be safer to have them contained within a namespace or a class (only if it belongs in a class).

      namespace vals
      {
          const int WORLD_HEIGHT = 4;
          // ...
      }
    

    The constant can then be accessed via the scope operator (::):

      int height = vals::WORLD_HEIGHT;
    

    Note(Note: The name vals is only an example, so feel free to give a more relevant name. As a user may be using it frequently (unless using is used), try to make it short.)

    If it does belong in a class, then make it a static const type (it must be static as it belongs to the class itself and not the individual objects). If it's an integer type, it can be initialized inside the class declaration as public, otherwise it'll have to be done in the implementation file.

    The scope access concept still applies, except you'll use the class name:

      int height = Class::WORLD_HEIGHT;
    
  • You can omit the ChunkManager constructor since you aren't already overwriting it. The compiler will provide a default one for you.

    Also, void parameters are not needed in C++, unlike in C. The compiler will already know that such call takes no arguments.

  • It's a little confusing to have multiple public and private sections. Just group them into one.

    Also, centerLoaded should be private as it is a data member. That should not be exposed to the public interface.

  • It's usually preferred to have your own headers before library headers. This can help avoid possible dependencies and also keep the headers more organized.

  • Do not use using namespace std in a header file. It can be okay to have it in an implementation file due to the locality (.cpp files do not get imported), but having it in a header file will force users to have the std namespace exposed, which can possibly break their code. Just leave it out and use std:: where appropriate.

  • Macro constants (or macros in general) are not very common in C++, and are primarily discouraged as there are better ways of doing things. Specifically, the const keyword should be used to make something constant and prevent any further changes to it.

      const int WORLD_HEIGHT = 4;
    

    Moreover, as these are in a header file, they will be global by default. While it is not such a bad thing since they cannot be modified, it would be safer to have them contained within a namespace or a class (only if it belongs in a class).

      namespace vals
      {
          const int WORLD_HEIGHT = 4;
          // ...
      }
    

    The constant can then be accessed via the scope operator (::):

      int height = vals::WORLD_HEIGHT;
    

    Note: The name vals is only an example, so feel free to give a more relevant name. As a user may be using it frequently (unless using is used), try to make it short.

  • It's a little confusing to have multiple public and private sections. Just group them into one.

    Also, centerLoaded should be private as it is a data member. That should not be exposed to the public interface.

  • It's usually preferred to have your own headers before library headers. This can help avoid possible dependencies and also keep the headers more organized.

  • Do not use using namespace std in a header file. It can be okay to have it in an implementation file due to the locality (.cpp files do not get imported), but having it in a header file will force users to have the std namespace exposed, which can possibly break their code. Just leave it out and use std:: where appropriate.

  • Macro constants (or macros in general) are not very common in C++, and are primarily discouraged as there are better ways of doing things. Specifically, the const keyword should be used to make something constant and prevent any further changes to it.

      const int WORLD_HEIGHT = 4;
    

    Moreover, as these are in a header file, they will be global by default. While it is not such a bad thing since they cannot be modified, it would be safer to have them contained within a namespace or a class (only if it belongs in a class).

      namespace vals
      {
          const int WORLD_HEIGHT = 4;
          // ...
      }
    

    The constant can then be accessed via the scope operator (::):

      int height = vals::WORLD_HEIGHT;
    

    (Note: The name vals is only an example, so feel free to give a more relevant name. As a user may be using it frequently (unless using is used), try to make it short.)

    If it does belong in a class, then make it a static const type (it must be static as it belongs to the class itself and not the individual objects). If it's an integer type, it can be initialized inside the class declaration as public, otherwise it'll have to be done in the implementation file.

    The scope access concept still applies, except you'll use the class name:

      int height = Class::WORLD_HEIGHT;
    
  • You can omit the ChunkManager constructor since you aren't already overwriting it. The compiler will provide a default one for you.

    Also, void parameters are not needed in C++, unlike in C. The compiler will already know that such call takes no arguments.

  • It's a little confusing to have multiple public and private sections. Just group them into one.

    Also, centerLoaded should be private as it is a data member. That should not be exposed to the public interface.

added 1112 characters in body
Source Link
Jamal
  • 34.9k
  • 13
  • 133
  • 237

Implementation file:

  • Do not keep srand() anywhere but in main(). It should only be called once so that the seed will not be reset at each call, giving you the same random values. Keeping it in the constructor is still not sufficient as you'll be creating multiple objects thus calling the function multiple times. By having it in main() only, you know that it will only be called once in the entire program.

  • Instead of using a constructor, you can use an initializer list:

      ChunkManager::ChunkManager()
          : renderDistance(8) //[5, 6, 7, 8, 12, 13, 14, 15, 20, 22]
          , centerLoaded(false)
          , timer(0)
          , timer2(0)
          , tickTimer(0)
          , removeTimer(0)
      {
          noises.push_back(new Noise(0, 0, 0, rand()));
          noises.push_back(new Noise(1, 0, 0, rand()));
          noises.push_back(new Noise(2, 0, 0, rand()));
      }
    

    Moreover, you don't need to allocate with new (this isn't Java). Leaving them out will still work as noises will then be adding anonymous objects (those initialized without a name). All it needs are the constructed objects, which you already have.

Implementation file:

  • Do not keep srand() anywhere but in main(). It should only be called once so that the seed will not be reset at each call, giving you the same random values. Keeping it in the constructor is still not sufficient as you'll be creating multiple objects thus calling the function multiple times. By having it in main() only, you know that it will only be called once in the entire program.

  • Instead of using a constructor, you can use an initializer list:

      ChunkManager::ChunkManager()
          : renderDistance(8) //[5, 6, 7, 8, 12, 13, 14, 15, 20, 22]
          , centerLoaded(false)
          , timer(0)
          , timer2(0)
          , tickTimer(0)
          , removeTimer(0)
      {
          noises.push_back(new Noise(0, 0, 0, rand()));
          noises.push_back(new Noise(1, 0, 0, rand()));
          noises.push_back(new Noise(2, 0, 0, rand()));
      }
    

    Moreover, you don't need to allocate with new (this isn't Java). Leaving them out will still work as noises will then be adding anonymous objects (those initialized without a name). All it needs are the constructed objects, which you already have.

Source Link
Jamal
  • 34.9k
  • 13
  • 133
  • 237

Header file:

  • It's usually preferred to have your own headers before library headers. This can help avoid possible dependencies and also keep the headers more organized.

  • Do not use using namespace std in a header file. It can be okay to have it in an implementation file due to the locality (.cpp files do not get imported), but having it in a header file will force users to have the std namespace exposed, which can possibly break their code. Just leave it out and use std:: where appropriate.

  • Macro constants (or macros in general) are not very common in C++, and are primarily discouraged as there are better ways of doing things. Specifically, the const keyword should be used to make something constant and prevent any further changes to it.

      const int WORLD_HEIGHT = 4;
    

    Moreover, as these are in a header file, they will be global by default. While it is not such a bad thing since they cannot be modified, it would be safer to have them contained within a namespace or a class (only if it belongs in a class).

      namespace vals
      {
          const int WORLD_HEIGHT = 4;
          // ...
      }
    

    The constant can then be accessed via the scope operator (::):

      int height = vals::WORLD_HEIGHT;
    

    Note: The name vals is only an example, so feel free to give a more relevant name. As a user may be using it frequently (unless using is used), try to make it short.

  • It's a little confusing to have multiple public and private sections. Just group them into one.

    Also, centerLoaded should be private as it is a data member. That should not be exposed to the public interface.