Game Engine using SDL 2

Started by
9 comments, last by kovacsmarcell99 9 years, 4 months ago



Hey everyone! I've done some work on the engine in the last half year currently I am adding the .cpp files. Could you guys give some further tips? (Hint: I've redone the logging pls check it. Is it good?)

Sure, here's a few things I noticed:


	//TODO: This MAY cause problems!
	bool isOpen = (bool)(in);

See http://www.cplusplus.com/reference/fstream/ifstream/is_open/


	Logger() {};
	Logger(Logger const&){};
	Logger& operator=(Logger const&){};

If your compiler supports C++11, consider using = default; or = delete; See http://en.wikipedia.org/wiki/C%2B%2B11#Explicitly_defaulted_and_deleted_special_member_functions


	static Logger* Instance();
	static void ResetInstance();

Consider making a templated Singleton class so you don't need to re-write this for every singleton you have. Personally, I try to stay away from them but sometimes they're appropriate (ie. a Logger class is quite appropriate).


#define LOG(Message_) Logger::Instance()->LogLine(static_cast<std::ostringstream&>(std::ostringstream().flush() << Message_).str());

Using a preprocessor to wrap any debug/log statements is a great idea, as you can easily have the #define statement do nothing for certain builds. However, consider utilizing the __FILE__ and __LINE__ macros to see where messages are popping up. This will help in the long run. (note that __LINE__ is an int).


inline std::string GetClockTime(void)

Consider making global functions as static functions in a class so you don't pollute the global namespace, and I don't seem to see any namespaces anywhere. Also, I assume this is C++ since you're using C++ tools, so remove (void) as it does nothing in C++ and only clutters things.


if (logNumber > 500)

500 is a magic number to me. Consider creating a constant that makes more sense to an external viewer.


	if (!instance)
		instance = new Logger();

At first, I was a believer of the "no braces for a single statement if statement" but unfortunately, it can be error prone (an error may only come up 1 in 1000 of these situations, but even that small chance is worth the coding style change), so consider always wrapping statements in { } braces.


	instance = NULL;

Consider using nullptr. See http://en.wikipedia.org/wiki/C%2B%2B11#Null_pointer_constant


Logger* Logger::Instance()
{
	if (!instance)
		instance = new Logger();

	return instance;
}

Consider returning a reference for a function if it's guaranteed that the item it returns will always exist. Only use a raw pointer if you cannot guarantee that the returned item will exist.

I also implemented my log/debug functions as functions that take a std::string as a parameter, but I probably should have just overloaded the << operator instead, so you may want to try that out as well.

I was just looking in TextureManager as well, and for your sanity, use typedefs or using = SomeC++11Type or using = SomePreC++11Type. I'm referring to this:



#ifdef CPP11_SUPPORT
	std::unordered_map<std::string, SDL_Texture*> textures;

	std::unordered_map<std::string, bool> errorShown;
#else
	std::map<std::string, SDL_Texture*> textures;

	std::map<std::string, bool> errorShown;
#endif

I hope this helps.

Thank you very much!

This topic is closed to new replies.

Advertisement