Check out my Engine!

[AndrewB]

Hey Guys,

I've decided to scrap what I have on my current project and start again. I'm making the new project open source to hopefully force myself to think about other developers and keep my code neat and readable.

Here is a link to the GitHub project: https://github.com/AndrewBerry/R9/
First commit of the engine working: https://github.com/AndrewBerry/R9/tree/cd4a3547cec9e1312b9c35db66a7ba3838ed6850

Any feedback is greatly appreciated, I've looked over this code a million times -- it would really help to get a second pair of eyes on it.

Thanks, Andrew

[Grimshaw]

Hmm, can we see some screenshots of what it can do instead?

[Nexus]

Some tips:

• c class prefixes are questionable -- simply omit them, the class names are descriptive enough
  
• Use constructor initializers list instead of assignments
• Perform initialization in constructor instead of public Initiate() method
  
• Use RAII (std::unique_ptr) instead of manual memory management (new/delete)
  
• Consider the rule of three (no need to declare destructor with RAII)
• Methods don't need to end with semicolon
• Identifiers like __ENGINE_HPP__ are reserved (two underscores or underscore + uppercase letter is forbidden)
  
• Don't include whole <SFML/Graphics.hpp> header, rather the specific parts you need
  
• Some headers are not used, e.g. <iostream>
  
• Use forward declarations to reduce compile-time dependencies
• For better encapsulation, make variables private instead of protected; use protected methods
• void in empty parameter lists is unusual in C++ (it's a relict from C)
  
• Avoid static variables in functions if you actually need member variables
• Types are not consistently named (exitData_t vs. cEngine) -- I wouldn't try to follow the standard library with _t postfixes
  
• Use type inference: replace std::map< std::string, cGenericScene* >::iterator with auto
  
• Avoid C and function-style casts, especially where no conversion is necessary: exitData = cGenericScene::exitData_t( cGenericScene::EXIT_CONTINUE );

[AndrewB]

Some tips:

• c class prefixes are questionable -- simply omit them, the class names are descriptive enough
  
• Use constructor initializers list instead of assignments
• Perform initialization in constructor instead of public Initiate() method
  
• Use RAII (std::unique_ptr) instead of manual memory management (new/delete)
  
• Consider the rule of three (no need to declare destructor with RAII)
• Methods don't need to end with semicolon
• Identifiers like __ENGINE_HPP__ are reserved (two underscores or underscore + uppercase letter is forbidden)
  
• Don't include whole <SFML/Graphics.hpp> header, rather the specific parts you need
  
• Some headers are not used, e.g. <iostream>
  
• Use forward declarations to reduce compile-time dependencies
• For better encapsulation, make variables private instead of protected; use protected methods
• void in empty parameter lists is unusual in C++ (it's a relict from C)
  
• Avoid static variables in functions if you actually need member variables
• Types are not consistently named (exitData_t vs. cEngine) -- I wouldn't try to follow the standard library with _t postfixes
  
• Use type inference: replace std::map< std::string, cGenericScene* >::iterator with auto
  
• Avoid C and function-style casts, especially where no conversion is necessary: exitData = cGenericScene::exitData_t( cGenericScene::EXIT_CONTINUE );
  

Thanks Nexus, I've read over what you've said and changed what I have.

https://github.com/AndrewBerry/R9/tree/580c69f6e885e07567a180407235aec5475b2cc5
How's that look?

[zsbzsb]

You still need to consider dropping the new and delete statements. For example change this in your tengine...

private:
    sf::RenderWindow *renderWindow;
public:
tEngine::tEngine() :
    currentScene( nullptr ),
    renderWindow( new sf::RenderWindow( sf::VideoMode( 800, 600 ), "R9" ) )
{
};
 

to

private:
    sf::RenderWindow renderWindow;
public:
tEngine::tEngine() :
    currentScene( nullptr ),
    renderWindow( sf::RenderWindow( sf::VideoMode( 800, 600 ), "R9" ) )
{
};
 

[Nexus]

You misunderstood me with the prefix, "t" is not any better than "c". You need no prefix at all, types are already uniquely recognizable because they are written in uppercase letters. Functions have the same naming convention, but are recognizable because of the parentheses. Therefore, simply use Engine, GenericScene, etc.

You still use manual memory management. Use automatic variables, and -- where necessary for polymorphism -- std::unique_ptr. Raw pointers should never own their objects. You can do it as follows. If you want, create a typedef for the unique pointer.

class Engine {
    ...
private:
    std::map< std::string, std::unique_ptr<GenericScene> > scenes;
    sf::RenderWindow                                       renderWindow;
    std::unique_ptr<GenericScene>                          currentScene;
};

...
Engine::Engine() :
    currentScene( nullptr ),
    renderWindow( sf::VideoMode( 800, 600 ), "R9" )
{
}

There are still some suggestions not applied yet (e.g. reserved identifiers, rule of three, static...), maybe read the list carefully again

[FRex]

Identifiers like __ENGINE_HPP__ are reserved (two underscores or underscore + uppercase letter is forbidden)

This is surprisingly common, Doom 3, Irrlicht, Ogre, I'm sure there is much more.

[eXpl0it3r]

This is surprisingly common, Doom 3, Irrlicht, Ogre, I'm sure there is much more.

So is a lot of other bad practice in programming with C++.

[Nexus]

eXpl0it3r is right. Common doesn't imply correct.

I don't know who initially used __MACROS_LIKE_THIS__, but it seems like bad ideas spread much easier than good ones. Same for Hungarian Notation, manual memory management, performance myths, STL myths, wheel reinventions, singletons, ...

As already mentioned elsewhere, Irrlicht and Ogre are not necessarily good examples for clean code. At least not in 2013. C++ is one of the few programming languages where paradigms and idioms have fundamentally changed during the last decade.

[FRex]

I know but many people(including cs teachers and students) don't care(or know) about things like that, it works here(ie. on one compiler, one system), the end, passed the class, hooray I'm a c++ programmer! Eh.. I've seen someone take out all deletes(because of course the course can't just go with RAII instead of state-of-the-art-of-90s c++98 and c89 and manual memory managment) out of his program to get under passing time threshold. I have to admit that's a killer idea that never crossed my mind... Of course everything is self cooked, often even STL is discouraged, because it's to test student's knowledge, not the fact they can use standard library or other libraries.. Maybe I just had bad experiences but no one checks students code for semi-arcane voodoo bullshit or they check it in a bitching way about lack of comments(seriously? that actually was about my reimplementation of std::vector(there was 50% points cut for using STL), I WONDER WHAT DOES getSize() and pushBack() do.. ffs..), style(padding with spaces or not), naming conventions(ANY is good as long as it's consistent within entire codebase) etc. On the plus side, hopefully c++11 and 16 will slowly keep growing and in the end there will be tons of sub standard programmers that write comments like (int i=0;//new local int i, initialized to zero) and can't use literally any library and self cook inefficient unsafe solutions, which will make us the gurus of the time.
I get daydreamy lately..

[Atani]

On the plus side, hopefully c++11 and 16 will slowly keep growing and in the end there will be tons of sub standard programmers that write comments like (int i=0;//new local int i, initialized to zero) and can't use literally any library and self cook inefficient unsafe solutions, which will make us the gurus of the time.

You must be new here ...   It has already started.. If you look at the general code quality produced today by recent grads vs seasoned devs you will notice a serious lack of standards.. However, I would love to see comments like that vs none at all!  At least that comment would show the dev knows something about what their program is going to do.

Isn't the pattern of:

#ifndef FOO
#define FOO
... code ...
#endif

in headers a bit of a relic of C days that was replaced with patterns like:

#pragma once
... code ...

[binary1248]

Testing a student's aptitude and teaching them how to program efficiently should not be done at the same time. At least at my university, we have at least 5 modules on programming/software development, each refining the knowledge of the previous one. Going higher and higher through the layers of abstraction. Squeezing all that content into a single module leads to that famous "implement your own list even though STL has one" scenario. We got to implement our own lists too, but in the context of C not C++ or any other OOP language the provides them already. Care was taken to also make sure no bad habits are created, penalizing us for not free()ing memory in C etc. although it would not have made a difference to a lazy programmer.

I don't know whether I just got lucky to be at my university, but I assume that given enough effort and good will, even with the crappiest courses you can become a pretty decent software developer if you care enough. The reverse is also true, even if you have the best courses in the world but don't give a damn about clean code, god help you.

The funny thing at universities is that, in the courses for students, they preach all the good stuff, but if you look at the code they use as part of their research you can hardly believe it was them who wrote it . Must be the time/financial pressure . This is probably also the reason all the bad stuff you listed made it into those other libraries. They wanted to get something out the door fast, no matter what. And since refactoring takes time away from "productive development" nobody ever cares once the codebase is there. Yes, management often doesn't care about RAII, and don't even try convincing them, you are better off refactoring yourself without their consent.

Isn't the pattern of:

#ifndef FOO
#define FOO
... code ...
#endif

in headers a bit of a relic of C days that was replaced with patterns like:

#pragma once
... code ...

That is a tricky one, because technically, #pragma once isn't standardized so if you want to get code to compile on some arcane (but standards compliant, like that even exists) compiler, you are safer using the first variant.

[eXpl0it3r]

... cs teachers and students ... course ... student's ...

Yep you should get back on ground, the whole world does not evolve around univerity classes. I'd claim that only a small percentage of the notable programmers have learned actual programming in the uni. The job of CS classes is not to teach you how to program for the industry, but it's a way to get back around to the theoretical parts, so you understand what's going on and getting into the whole "thinking in binary".
Sure it would be nice if the profs would up their style and knowledge so that the students could fully profit from the course, but it's necessary for the given task.
Besides that, it's impossible to teach C++ in a good manner while keeping up with the actual topic at hand in one semester, unless you'll have a "Introduction to programming" course for every single programming language

Isn't the pattern of:

#ifndef FOO
#define FOO
... code ...
#endif

in headers a bit of a relic of C days that was replaced with patterns like:

#pragma once
... code ...

I have to disagree! While the #ifndef #define #endif pattern is accepted by the C++ standard #pragma once is not and you'll have no guarantee that your compiler is supporting that feature. True it's much nicer to see and write and as a matter of fact I'm using it as well, but it's no where near an indicator for better written code.

[Atani]

I have to disagree! While the #ifndef #define #endif pattern is accepted by the C++ standard #pragma once is not and you'll have no guarantee that your compiler is supporting that feature. True it's much nicer to see and write and as a matter of fact I'm using it as well, but it's no where near an indicator for better written code.

By far, it does not make the code any different between the two options outside of 2+ fewer lines.

That is a tricky one, because technically, #pragma once isn't standardized so if you want to get code to compile on some arcane (but standards compliant, like that even exists) compiler, you are safer using the first variant.

Now, re support of this in the compiler/standards..  It is not part of the standard (I thought they put it on a list for Cx10/11 but its not there so oh well)... However, a lot of the compilers out there support it... I would advocate using it over the header guards since the compiler can optimize away recompilation of the unit (not all compilers do that optimization though).

[netrick]

Some tips:

• Use constructor initializers list instead of assignments

Why? Consider this C++11 example.

class example
{
public:
void foo();
private:
int a = 0;
bool b = false;
std::string name = "default";
};
 

Faster and easier solution than initializers lists. C++11 is a big step forward. Using that, if you want to check/change the value you simply look at the variable declaration. And you don't produce unnecessary boilerplate code.

Of course there are cases when you must use initializers list, but generally I think that assignments are better.