The idea of MVC is nice, there have also been several game engines for SFML that use a similar approach. But I always wonder how scalable and generic these are, because most are tailored for a specific game and have specific requirements.
Also, ideally the model wouldn't have any SFML-related data or functionality at all (except maybe time and vectors), which doesn't seem to be the case for your pong example. Actually, I see no separation of graphics and logics at all. What I would consider separate is, for example:
- A class for logics and one for graphics. The logics class holds an opaque pointer to the graphics class, and a renderer reads it
- A class for logics and one for graphics, but they don't reference each other directly, but through a map with an index or identifier
- A logic class and a renderer class that has draw() overloads for different logic objects, and constructs the graphics on-the-fly
This suggestion of wrapping SFML, of abstracting away its coupling of "logic" and "presentation", and just assuming that I'm able to do such a thing, is quite a compliment. Thanks!
However, while I've done similar small frameworks before, I don't have the time to go into that, sorry. Also I think it would be a good idea with some experience in games programming before trying to address the problems of that area, or even guessing what the problems are! And while I know a lot about general GUI programming, C++ and software development in general, I know next to zilch about games programming.
I agree that the code is very hard to read. That has to do with the unusual placement of newlines and whitespaces, as well as the use of under_lines convention, which makes it more difficult to recognize identifiers as a whole (especially in the presence of operators).
Example:
inline
auto sign( sf::Time const& time )
-> int
{ return (time < sf::Time()? -1 : time == sf::Time()? 0 : +1); }
vs.
inline int sign(sf::Time time)
{
return time < sf::Time::Zero ? -1
: time == sf::Time::Zero ? 0
: +1;
}
(or use an if or the non-conditional sign trick).
Furthermore, you're very inconsistent with the placement of curly braces; sometimes everything is on one line, sometimes you distribute it across multiple lines with braces on their own line [...]
As it happens the spacing and naming convention I use is mostly the same as in the standard library and Boost library code, modulo my use of
auto function syntax. And there is no alternative to
auto function syntax for lambdas and for some template functions, so one can't avoid having to deal with it. So if any of this feels unreadable or uncomfortable, then I think it can be a good idea to force oneself to get used to it, so as to be able to deal effectively with "standard" library code and modern code using lambdas etc.
But I do now tend to write a one-liner body on a single line, yes.
As I see it that's not "inconsistent". On the contrary, running the code through the free
AStyle code formatter with option
style=--allman, which generally ensures consistency, retained all the one-liner function bodies. AStyle did however replace one one-liner statement
if( n_open_windows == 0 ) { break; } with four lines and indenting, which to my eyes reduce the readability of the function as a whole -- but I guess it depends on what one's used to. ;-)
Anyway, I recommend
AStyle or some similar formatting tool to deal with formatting that one finds uncomfortable, or that is simply inconsistent or lacking. Nowadays even Visual Studio has code formatting. And both AStyle and Visual Studio's formatting can be configured to your liking via a host of options.
, sometimes [the curly braces are] in the middle of a multi-line block...
I suspect that here you're misinterpreting C++11
curly braces initializers.
Other than that, the code looks quite good, maybe some minor remarks:
- Don't use top-level const qualifiers for parameters. In char const* const title, the second const is useless, as it only says that the copy of the pointer (local to the function) may not be changed. Same for sf::Time const time_now, don't overuse const where it doesn't contribute to semantics.
- Use static_cast instead of C casts
- Instead of giving classes like Point a default template parameter for float, I'd rather do it like SFML and provide typedefs for int and float. Something like Point<> that is used so often seems weird.
• Regarding
const,
the same rationale that applies for using
const on local variables, applies to using it at top level for formal arguments. Namely, it reduces the number of things that can vary and that one has to keep track of for understanding the code. Note that for ordinary application programming this is the sole purpose of
const.
I fail to understand what you mean by "doesn't contribute to semantics", sorry.
Maybe you meant "doesn't contribute to the function's formal type" (signature plus return type), which is technically true, but which I then don't understand the relevance of?
• Regarding use of
static_cast,
you're right that C style casts set a bad example, even when they're "safe". I should not have used just
(float) to suppress sillywarnings, mea culpa! However,
static_cast gets a bit too verbose for my taste in such cases, so (when thinking about it) I would rather choose to e.g. multiply by
1.0f or add
0.0f just to force a conversion.
• Regarding default parameter for templates,
I did consider a typedef like
Point_f. However, the syntax
Point<> very clearly says that this is
the default, the natural choice for the library, while
Point_f is no different from e.g.
Point_i. Besides, I'm adverse to type-prefixes and suffixes... ;-)
Cheers, & thanks for you feedback!,
- Alf