Fake 3D effect in SFML - follow up

Based on my previous question, I have implemented all the recommendations received.

Here is a summary of the improvements:

• Use meaningful names as much as possible.

• Removed multiple declarations in one line.

• Removed the dynamic_casts and applied a new class hierarchy design.

• Added a Game class to improve code modularity.

I would like to know whether or not I have implemented the class hierarchy design for Colors in a suitable way.

```
#include
#include
#include
#include
#include
#include

namespace
{
float increase(float start, float increment, float max)
{
auto result = start + increment;

while (result >= max)
result -= max;

while (result TimePerFrame)
{
timeSinceLastUpdate -= TimePerFrame;

processEvents();
update(TimePerFrame);
}

render();
}
}

private:
void processEvents()
{
sf::Event event;
while (mWindow.pollEvent(event))
{
if (event.type == sf::Event::Closed)
mWindow.close();
}
}

void update(sf::Time timePerFrame)
{
auto dt = timePerFrame.asSeconds();
auto maxSpeed = mSegmentLength / dt;
auto accel = maxSpeed / 5.f;
auto breaking = -maxSpeed;
auto decel = -accel;
auto offRoadDecel = -maxSpeed / 2.f;
auto offRoadLimit = maxSpeed / 4.f;

const auto& playerSegment = *mSegments[static_cast(std::floor(mPosition / mSegmentLength)) % mSegments.size()];
auto speedPercent = mSpeed / maxSpeed;
auto dx = dt * speedPercent;

if (sf::Keyboard::isKeyPressed(sf::Keyboard::Left))
mPlayerX -= dx;

if (sf::Keyboard::isKeyPressed(sf::Keyboard::Right))
mPlayerX += dx;

if (sf::Keyboard::isKeyPressed(sf::Keyboard::Up))
mSpeed += accel * dt;
else
mSpeed += decel * dt;

Like I commented, It looks pretty clean overall, so I'll only mention a couple things this time. You might consider waiting a little bit before selecting this, in case other reviewers show up.

-
mSelf in Colors is strange. I didn't quite understand its purpose and what you are trying to achieve with it that can't be accomplished with the this pointer. I think you might have done that because of the getColor/setColor methods? That's not a very conventional approach. The usual would be for Segment to have a Colors pointer, then just swap that pointer when you need to setSegmentColors.

void setSegmentColors(const Colors * c) { mColors = c; }

-
The methods of Colors could probably be pure virtual (= 0;). I don't suppose you'd need to declare a Colors instance by value, one the mSelf oddity is fixed.

-
When you implement a virtual base class, be sure to annotate the methods in child classes with override.

-
Also take a look into the final specifier. You can add it to classes that are not meant to be inherited from and to leaf nodes in a class hierarchy.

-
All the virtual get* methods in Colors should be const. They are not mutating any member data.

-
The globals light and dark, even though not carrying any member state as of now, should also probably be constants. I'm very picky about const because mutable shared state is a major source of bugs. Knowing a thing is const greatly reduces the number of places you have to look into if you find an inconsistent state somewhere.

-
No need to call the default constructors of all members of Segment in its constructor. This is C++11, so if you'd like to emphasise that they are default initialized, you can just use the {} syntax on declaration, e.g.: Point mPoint1{};. If you do that, you can get rid of Segment's constructor.

-
rumbleWidth() and laneMarkerWidth() could either be joined with the other helpers inside the unnamed namespace or could be declared as static, since those are pure functions that are not relying on any member state of Segment.

StackExchange Code Review Q#111973, answer score: 2