Refactorisation/tidying up

[mspraggs]

This is a more long term goal, but it'd be good to refactor and neaten things up to improve maintainability. Some ideas:

• Consistent naming convention. (Camel case?)
• Comments where needed.
• Improve encapsulation.
• Const correctness.
• Implement a model-view-controller paradigm, i.e. separate game logic and graphics as much as possible.

[DivFord]

For that last one, we need an asset loading script, rather than putting the file paths for textures and sounds in player, and wherever prop and block tile sheets are being loaded. We can then just initialize it in game, and include it in anything using textures or sounds.

[mspraggs]

I agree, there could potentially be an Asset class with a constructor that loads all asset data from a file with a consistent format, something like a json, csv or plain text file. On 26 Jan 2015 12:23, "DivFord" notifications@github.com wrote:

For that last one, we need an asset loading script, rather than putting the file paths for textures and sounds in player, and wherever prop and block tile sheets are being loaded. We can then just initialize it in game, and include it in anything using textures or sounds.

— Reply to this email directly or view it on GitHub https://github.com/mspraggs/GameJam2015/issues/6#issuecomment-71453058.

[DivFord]

Better encapsulation on Sprite would be nice. For example, rather than:

gate_.sprite().texPos().x = gate_.sprite().texDim().x;

we should be able to call something like:

gate_.sprite().animFrame(2);

[mspraggs]

This is the kind of thing I think we should aim for eventually: http://www.koonsolo.com/news/model-view-controller-for-games/

[Fyll]

The "texPos = texDim" thing would be easy to get around, I was just sleep deprived at the time. :P. There is a shunt(n) function that progresses the sprite n steps along its animation, but that wasn't appropriate when the thing may be called multiple times.

I'd be against leaving the maps in text files, and would rather just put them in a header/source file or something, so that the player doesn't need to see them externally. I've got a program that can put bmp's into headers, but I've never tried it on png's, so don't know if that's applicable. Also, I don't know how to do anyhting with sound.

The camel caps is just a habit of mine. We can go either way.

The const correctness I'm not sure I understand. It's just never something that has ever been a problem for me, so I've never used it. (I'm assuming you mean how some of your functions were const and took const parameters).

On separating gameplay and sprites, the only way I think that comes up in the code at the moment is when considering sprite.dim() and when forcing sprites (as in the "texPos = texDim" problem above). I personally think of the Sprite class as the barrier between gameplay and graphics, so don't really see the problem.

[DivFord]

I'm not sure I understand what you mean with putting pngs into headers. All we really want is to have all the file paths as constants in one header so we can edit them centrally.

[Fyll]

Oh, I misinterpreted (although I am still in favour of absorbing all of the images, etc. into the executable).

Putting them in a header should be a doddle if anyone wants to do it.

[mspraggs]

Const correctness is basically a way to reduce errors in code by ensuring that certain variables can't be modified. They're most important in the case when you're passing around pointers or references, since they prevent the accidental modification of the object the reference/pointer points to. The classic example is if you want a get method for a member variable that's a large object. Returning by value would be expensive, since a memory copy would be required. You can remove problem this using return by reference, but then the problem is the function could potentially be used to assign to the member variable through the reference (a set method makes more sense in this case, or just making the variable public). Putting const at the end of the function definition/declaration tells the compiler that this function can't modify the class, thus removing the danger of assignment.

With the MVC the idea is that by decoupling the graphics from the game logic, if the day ever comes when you want to completely redo one or the other (in our case maybe something like changing the screen resolution or something), then you don't have to disentangle the relevant bits of code in order to make the change. What you do is encapsulate the game physics and other logic in model classes, the graphics/output code in view classes, then a controller class is used to handle user input then update the game and graphics states.

I suppose one of things that prompted me to suggest the MVC approach is how we use graphics events to trigger various behaviour in the game, such as window fades. Ultimately we wouldn't rely on the graphics state to control the game, since this could result in code that has a high level of dependency amongst individual functions and files. MVC has the added advantage that the game logic and graphics can be updated independently and at different rates.

I quite like the way levels can be constructed using pure ASCII files, but I understand your desire to hide this from the user. Perhaps the solution is to have a second executable that can render the assets into some format that obscures their content, and this can then be loaded by the main game code. That way whenever we create a new asset, we run it through this obfuscator, then commit both the "source" asset and the obfuscated one.

[DivFord]

The screen fading thing has definitely caused at least one bug already, and may be responsible for another.

Agreed on the ASCII files part. They may be causing a bug in the application when it's not run from the terminal though, so an executable to convert them to a header might be nice… Code that writes code?

[Fyll]

I'd forgotten about the screen fading thing. I'll admit that that was just a quick way to tell if the screen is blank, in order to know when to move on to the next level. Seeing as Game is now an instance, I could just put a function in there which checks thw screen, then make everythign else check that function.... Actually, I quite like this idea. I'll do it now.

All it would need to be is a const array of strings, in a similar style to the guns and props.

[mspraggs]

This book looks pretty useful: http://gameprogrammingpatterns.com/

There's an online version that's completely free.

[mspraggs]

The functions you've written in Game.cpp look good. Perhaps we could have Game query the graphical state, then manipulate the World? That way the World isn't directly querying the graphical state.

[Fyll]

I'll give it a go. As an aside, do you think the movement is smoother now? I've changed the jumping and running to work from acceleration rather than velocity, and feel it's better for it.

[mspraggs]

I'm going to go through the source code and try and clean up the class interfaces and enforce C++ best practice/standards. To do this I'll follow Scott Meyers' Effective C++ and Effective Modern C++. I'll post the chapter titles for reference when I have the books to hand.

[mspraggs]

Effective C++:

1. View C++ as a federation of languages.
2. Prefer consts, enums and inlines to #defines.
3. Use const whenever possible.
4. Make sure that objects are initialised before they're used.
5. Know what functions C++ silently writes and calls.
6. Explicitly disallow the use of compiler-generated functions you do not want.
7. Declare destructors virtual in polymorphic base classes.
8. Prevent exceptions from leaving destructors.
9. Never call virtual functions during construction or destruction.
10. Have assignment operators return a reference to *this.
11. Handle assignment to self in operator=.
12. Copy all parts of an object.
13. Use objects to manage resources.
14. Think carefully about copying behaviour in resource-managing classes.
15. Provide access to raw resources in resource-managing classes.
16. Use the same form in corresponding uses of new and delete.
17. Store newed objects in smart pointers in standalone statements.
18. Make interfaces easy to use correctly and hard to use incorrectly.
19. Tread class design as type design.
20. Prefer pass-by-reference-to-const to pass-by-value.
21. Don't try to return a reference when you must return an object.
22. Declare data members private.
23. Prefer non-member non-friend functions to member functions.
24. Declare non-member functions when type conversions should apply to all parameters.
25. Consider support for a non-throwing swap.
26. Postpone variable definitions as long as possible.
27. Minimize casting.
28. Avoid returning "handles" to object internals.
29. Strive for exception-safe code.
30. Understand the ins and outs of inlining.
31. Minimize compilation dependencies between files.
32. Make sure public inheritance models "is-a."
33. Avoid hiding inherited names.
34. Differentiate between inheritance of interface and inheritance of implementation.
35. Consider alternatives to virtual functions.
36. Never redefine an inherited non-virtual function.
37. Never redefine a function's inherited default parameter value.
38. Model "has-a" or "is-implemented-in-terms-of" through composition.
39. Use private inheritance judiciously.
40. Use multiple inheritance judiciously.
41. Understand implicit interfaces and compile-time polymorphism.
42. Understand the two meanings of typename.
43. Know how to access names in templatized base classes.
44. Factor parameter-independent code out of templates.
45. Use member function templates to accept "all compatible types."
46. Define non-member functions inside templates when type conversions are desired.
47. Use traits classes for information about types.
48. Be aware of template metaprogramming.
49. Understand the behaviour of the new handler. 50 Understand when it makes sense to replace new and delete.
50. Adhere to convention when writing new and delete.
51. Write placement delete if you write placement new.
52. Pay attention to compile warnings.
53. Familiarise yourself with the standard library, including TR1.
54. Familiarise yourself with Boost.

I can't claim that I have read the entire book, or that I understand what all these points mean. Several of them are definitely not relevant to our project, at least at the moment. Some are definitely worth bearing in mind though.

I'll post the chapter titles from Effective Modern C++, which deals with C++11-specific issues, tomorrow.

[DivFord]

I hear bad things about virtual destructors (http://www.gamasutra.com/blogs/PiotrTrochim/20150204/235736/Virtual_mess.php), but other than that it sounds good.

[Fyll]

Wow, it looks like you've got your work cut out for you!

Yeah. I've learnt to program by explicitly doing things, so only know what I've had to do before, so don't understand half the things on the list either!

I do also just want to stress the un-importance of good practice. Even though a lot of those things may be good practice generally, unless they actually make the code better (in efficiency or readability), they probably should be ignored.

[mspraggs]

I'll have to read about virtual destructors. From what I've heard, if you don't make a base destructor virtual, he base class destructor won't be called when the child object is destroyed.

I think that the reason much of this advice has sprung up is that it reduces the probability that bugs will creep into the code and make it significantly easier to debug. If it were pointless, no one would do it.

[Fyll]

However, it's not all necessarily applicable. These'll be general rules, an what's better in one situation may be worse in ours.

Also, do pull before you start this, as I've made quite a few changes to Game.cpp.

[mspraggs]

Indeed, much of this doesn't apply to our code. The latter points about overriding new/delete for a start don't seem to bring anything to bear on this project, for example. I'm not planning on doing massive restructuring, just tidying up interfaces to make the code as a whole safer.

[DivFord]

Ooh. I like your changes Iain, you've got a lot done. I think the ice wand has broken though...

[Fyll]

Oh yeah, that'll teach me to quickly add things in. Fixed, and made more robust (when I added the signs as their own block type, it shifted water and ice one higher each, so when you froze water, it replaced it with block number 5, which was now water. I've changed it to work by code instead of index).

[mspraggs]

Ok I'm all up-to-date. I'll have a look at starting this later, once I've got some work done. I'm impressed by the progress!

[mspraggs]

As an aside do you two use Google chat/hangouts much? Would be a good way to discuss things generally without clogging up the Issues threads.

[DivFord]

I do occasionally. Seems like it might be a good idea.

[mspraggs]

Ok, I'm making reasonable progress with this. A few things I've focused on:

• const correctness
• Use of auto for automatic type inference
• Range based for-loops

I've also tidied up Object and its children a bit. Now instead of having various property variables scattered throughout the child classes, they're all member variables of Object.

I've also made Game::objects_ a vector of std::unique_ptrs, so we no longer need to worry about deleting Objects, and should get some garbage collection functionality. So now to delete all objects you just need to call objects_.clear(). To get the raw pointer, i.e. get an Object* to pass as an argument, for example, just do obj.get().

[Fyll]

Nice!

Just a quick question (I haven't looked into this), but isn't using auto less efficient than explicitly giving the type? Unless it's just for readability, but even then...

EDIT: Having looked into this, it seems it is just as efficient. However, I'm going to grumble a lot and claim that it makes everything harder to read (especially the Range loop if the sample code is anything to go by).

EDITx2: Having now looked at the code you pushed, I've got a few things to say:

Can we keep loops on one line? I know it may make it wrap on your screens (can't you disable text wrapping?), but it makes everything so much easier to read (for me at least) when the loop doesn't take up the same space as its contents.

What did you do to Object::objectAtPoint()? Suddenly, it looks so confusing. Are function pointers really that bad (they're not on your list)?

Why did you change all of the tabs to spaces? It's really hard to read now (especially with all of the broken up lines).

The compiler throws some errors. They all seem to be about hiding overloaded virtual functions.

[mspraggs]

Hmm... It builds fine for me. Is it possible you have local changes that are affecting things? Open an issue and post the output of your make command and I'll see if I can find the cause.

The auto keyword is supposed to reduce the amount you have to write by telling the compiler to automatically infer what type should be used at compile time. It's especially useful in situations with templates where it's difficult to determine what the type should be. In the case of for loops with iterators it's useful because you don't have to write out std::vector::iterator, you can just write auto and the compiler will figure it out. It's supposed to make things easier for the programmer by making him think less about what type a variable needs to be.

Range based for loops are supposed to simplify the following syntax:

for (std::vector<Foo>::iterator item = list.begin(); item != list.end(); ++list)

To this:

for (Foo item : list) // N.B auto keyword not needed, but sometimes saves typing

The idea being that any variable with a begin and end function can be used in this way. I think it streamlines the syntax significantly.

I find that keeping the code within a certain width makes it easier to read. Maybe 80 char width is too aggressive, and maybe we could relax this a bit. However, in much the same way that newspapers have columns for easier reading, keeping the code width to something sensible will also make it easier to read, and also may highlight bits of code that are too long/complicated and need to be refactored. On the other hand, I understand your gripe with ifs and for loop bodies spanning multiple lines. Perhaps we should limit our lines to around 120 chars? That way I think virtually all existing for loops will fit.

Ah yes, I have rather over-engineered objectAtPoint. Now instead of requiring a specific member function of Object, any function that takes a const Object* can be passed in. If we're never going to need this degree of generality, then we can of course go back to using function pointers. Alternatively, if the use of lambda functions looks too confusing (on Game.cpp:267), then we could change the instantiation of checkSolid to the following:

std::function<bool(const Object*)> checkSolid = &Object::solid;

This way it's obvious that checkSolid encodes a function pointer. Or, as I say, if we're only ever going to use Object member functions, we can change it back to how it was.

Tabs and spaces. Can I propose we settle for a fixed number of spaces for indenting (how does 4 grab you?) and just stick with that. Different editors render tabs differently, whereas spaces always come out the same, so everyone sees the same thing. If you're using gedit, you should have the option to tell to use spaces when you hit tab, if memory serves correct.

[Fyll]

It builds, it just throws a few warnings. Maybe you haven't pushed your latest version?

Here's my problem with auto/range loops: They reduce the code, not simplify it. I appreciate that it does simplify some of the longer lines, but some loops can't be condensed into a range (due to extra conditions), and the fact that we aren't using a consistent style for loops just seems to confuse things further. Specifically, this is a complaint against range loops rather than auto. Auto seems fine, even if I will have to bother to open up the header file whenever I want to find out if a vector contained instances or pointers.

Hmm. In my eyes, having a line of code go off of the edge of the screen is fine, seeing as I won't be looking at any particular line of code unless I'm specifically looking for it (if that makes sense), so having one long line that goes off of the edge of the screen helps me to find the other lines of code which, statistically, I'm more likely to be looking for (seeing as they vastly outnumber the long lines).

Maybe it's just my lack of understanding of lambda functions (you didn't even declare what types you were passing in to the template function! D:), and my understanding of function pointers, but I definitely would rather it was done with a function pointer (sorry). I'm pretty certain we'll never be accessing functions from other classes in there, as it was only meant to check if an object was there which obeyed a certain property. Also, isn't declaring a function on the heap a bad idea memory wise? Particularly compared to just using a function pointer.

4 sounds good. 1 was just a (fair) bit too few. And I've spotted the "use spaces" button, so will try to make sure that's turned on from now on.

[mspraggs]

It must a compiler-dependent thing then. If you post the warnings we can see what's going on.

If anything I'd argue that range based for loops are clearer than their normal for-loop counterparts, because once you're familiar with the syntax, it's immediately obvious what the for loop does. Compare this to normal for loops, where the conditions and incrementation behaviour can potentially be quite complex.

I think the indentation should be 2 spaces, but 4 seems a good compromise. Can we just keep it to around 120 chars? It may also reduce the likelihood of mismatched parantheses.

Sure, as I said we can move back to using function pointers if you're happier with that. When using a templated function you don't need to specify the template parameters, and this is one of the beautiful things about templates. I think the function would be declared on the stack, as it's not dynamically allocated, but even still this wouldn't cause a problem. Lambdas are just a nice way to create small functions that can be passed into functions and other similar things.

[mspraggs]

Okay I've switched to using clang and silenced those warnings. Basically when various classes were inheriting from Object they were redeclaring several functions defined virtually in Object with a different signature. Clang was warning that functions in the parent were being hidden by the definitions in the children. I've fixed this by specifying using Object::funcName; before each new function definition, to tell the compiler that we don't want to hide the previously defined function.

[Fyll]

/home/iain/Projects/David/GameJam/GameJam2015/lib/Player.hpp:21:8: warning: 'Player::draw' hides overloaded virtual function [-Woverloaded-virtual] void draw(Vector shift); ^ /home/iain/Projects/David/GameJam/GameJam2015/lib/Object.hpp:160:16: note: hidden overloaded virtual function 'Object::draw' declared here: type mismatch at 1st parameter ('const Vector &' vs 'Vector') virtual void draw(const Vector& shift) {

This happens for each of Object's children, for each of init, update, and draw.

Isn't the capacity for having more complicated conditions and increments in for loops a good thing? Comparing:

for(auto obj(objects_.begin()); obj < objects_.end(); ++obj)

for(auto obj : objects_)

Sure the range loop is more compact, but readability is an objective thing. Yes, if I got used to the syntax, the range loop would be easy to understand, but at the same time, I can see the std:: (or auto in this case) at the start of the for loop and immediately tell that it's iterating over the contents of a vector. This is why I'm arguing for consistency instead of readability, as you find the range loop more readable, but I find the long version more readable.

Again, each to their own, but I find being able to tell roughly how deep a line of code is at a glance to be very useful. Mismatched brackets should not be a problem, as the compiler flags them up straight away (and breaking the code onto multiple lines makes it harder to read, thus increasing the chance of runtime errors).

...Isn't that a bad thing though? Clarity is surely an important thing here.

But why are init, update, and draw virtual then? Init at least, the children don't want to inherit.

[mspraggs]

I think it's because Object::init is called in the child init methods. Making Object::init non-virtual would be a bad idea because then you'd be redefining the method in the child class. So the two options are either make init virtual in Object and have it non-hidden in the child, or call it something else, like initBase, and have this as non-virtual.

Why don't we just use both styles of for loop? We have that flexibility. Whenever we need a more complex exit condition, just use the traditional type.

Templates are supposed to take some of the weight off the programmer when defining functions, instead letting the compiler do the heavy lifting. If you wanted absolute clarity on what type was being passed to a function, you wouldn't use a template. Any compiler errors that results from templates normally specify the types the compiler is trying to use for the template anyway.

[Fyll]

Fair enough, although init didn't use to be virtual did it? Having them both called the same thing just means there are two initialising functions defined: init(child stuff) and Object::init(parent stuff). They still have different signatures, so it shouldn't be a problem.

But surely consistent notation is a good thing.

[mspraggs]

This isn't something I've changed (just looked at the git log). I'm not entirely sure why you're suddenly getting warnings. Perhaps it's because I made some other functions in Object non-virtual. I can't be sure. One thing we certainly mustn't do is this:

class Base
{
    void someFunction(args);
}

class Child : public Base
{
    void someFunction(other_args);
}

Now what happens when you do this?

Base* foo = new Child();
foo->someFunction(/* your args here */);

Because foo is a pointer to a Base object, we're actually calling Base::someFunction. Hence by redefining non-virtual function in the child class, you're losing all the advantages of polymorphism. I haven't tried this out for the case when Child::someFunction has different arguments to the parent, but I have a hunch that the compiler won't be able to resolve the function properly and it just won't compile.

[Fyll]

Having taken the virtuals out from Object, everything works fine. This is because everything is defined as a type of itself (that sounds wierd...). The initialiser functions are almost always called from the constructors, and are only otherwise called from the children themselves. I can see why the problem could exist, but as long as we're not using pointers like that, it shouldn't be a problem.

Update and draw are only overloaded by player, who is handled separately.

[mspraggs]

Okay. In that case we should make Object::init protected so that it can never be called using a base pointer. I'd also suggest renaming it to something else, like initBase, so that we're not hiding the function in the base class. If we were to redefine the function in the child class but keep the same name, we'd be doing away with the concept that an instance of Child is also an instance of Base, as we're redefining what Base in the case of Child. This would go against the grain of class inheritance.

In summary, the two options I can see are:

class Object
{
protected:
    void initObject(args); // protected, so ptrObj->initObject(blah) won't compile
};

class Child : public Parent
{
public:
    void init(moar_args)
    {
        Object::initObject(other_args);
        ...
    }
};

or

class Object
{
public:
    virtual void init(args); // virtual, so calling init on Object* will call init for correct class
};

class Child : public Parent
{
public:
    using Object::init; // Tells the compiler not to hide init in the base class
    void init(moar_args)
    {
        Object::init(other_args);
        ...
    }
};

EDIT: In fact, initObject no longer needs to be protected, as ptrObj->init will fail to compile anyway.

[Fyll]

Hmm. The first option would probably be best, seeing as it would ensure we never accidentally try to create a pure Object.

[mspraggs]

Yes I agree.

Out of curiosity, why are there so many init functions? Initialization is normally taken care of in the constructor. I've seen instances when we've reinitialized an object with some parameters, but is this true for all cases where init is implemented?

EDIT: Can I suggest that if the init functions are generally only used within their respective classes, then they be made protected or something. Again, reduces the possibility of abuse at a later date.

EDIT2: In C++11 you can actually have delegating constructors. E.g.:

class Foo
{
public:
    Foo() : Foo(1, 2, 3) {}
    Foo(const int a, const int b, const int c) : a_(a), b_(b), c_(c) {}

private:
    int a_, b_, c_;
};

This way we could replace some of the inits with constructors, and delegate to these instead of needing to call init. Then, if we ever need to reinitialise and object, we can just do *this = Foo();.

[Fyll]

Okay. I've pushed a version with all of the tabs to 4 spaces (actual spaces), and the vast majority of lines cropped to be < 120 characters (the exceptions are just the odd line hidden away in somewhere that shouldn't ever really be looked at again).

[mspraggs]

Great! I've removed the open function from Game and put the code into it's constructor.

There was a merge conflict when I pulled you changes, and I may have accidentally overwritten a few lines of your changes in Game.cpp, but otherwise I have the spaced version now.

[Fyll]

Oops, didn't see your post.

I think the only time anything is reinitialised is when a block changes type (water <-> ice). I can get behind making initialisers protected. I just wrote them as I did by force of habit.

[mspraggs]

Ok great. In some cases it may be possible to just use constructors and cut out the middle man.

I do seem to be having trouble with Player::init though. I put a call to Player::init in Player's constructor, and the results are... interesting. Much of the screen is rendered white. I'll investigate.

[Fyll]

Re-pushed with Game.hpp spaced properly again (and a few tweaks elsewhere).

EDIT: Either we're posting amazingly in unison, or git's playing up and not showing me your posts until I post.

[Fyll]

From your list: 9: Never call virtual functions during construction or destruction.

Player's init() calls Object's init(), and Object may not be properly created at that point (I'm not sure when Object's constructor gets called). Also, why are you doing that? Player gets initialised when the game is started by init() (and needs to be here, so he can be reset when you die).

[mspraggs]

Ah yes well spotted. It just concerns me somewhat having variables floating around uninitialised. Ultimately I suspect Object::init can be removed entirely and replaced by a protected constructor or something similar.

If we merged Player::init with its constructor, then you could simply do player_ = Player() each time the game is reset. What do you think?

[Fyll]

It is pretty much explicitly one of the first things called, so it's not floating around for long.

[mspraggs]

I know. I'm not suggesting these things to be awkward. I honestly believe that they'll make the code easier to maintain in the long run. Besides, in the majority of cases, init seems to basically our roll-your-own constructor. Why reinvent constructors when the language gives you one already?

[Fyll]

True, I'm just used to not being able to reinitialise something with the constructor, so wrote them out of habit.

I think (I've rather forgotten now >.<) my problem is that ideally, main calls Game::Game(), which calls Game::init() (can be renamed to reset() if that'd be better). Game::init() calls the various intialisers (world and player), setting up the new game. My worry is that Player::Player() will be called when Game is created, then also needs to be called when it is reset (via Game::init()).

[mspraggs]

Well... if the contents of Player::init are just moved to the constructor, whenever player_ needs to be reinitialised you just to add the line player_ = Player();. I'll give this all a go. If it doesn't work out or makes more mess we can always revert.