Sign in to follow this  
Flyverse

Improving at programming: Code Review

Recommended Posts

Heya!

 

While programming has been one of my hobbies for quite a few years now, I feel like I haven't been progressing a single bit these past two years.
Since I taught myself programming by simple trial and error, I'm obviously incredibly far away from a "professional" level - But I'd still have liked to get a bit better.

The thing is, that I don't even know what exactly I need to improve - And that's why I thought that it might be really helpful if someone could give a quick look at some of my more recent code, and maybe tell me what I could improve (Not really specific to the code itself, but more in general)

The problem is that I obviously can't just post loads of code, since it'd be way too tiring to look through that, so I'll try to just post a few snippets of some code that can hopefully be understood without context as well.

In any case, I'd truly appreciate if someone could give me a piece of advice to progress some more, going from the code samples below! Also, I'm really sorry for the length of this, but I couldn't find shorter... Really sorry! I totally understand if this is too much, but I didn't know how I could cut it down to only post snippets.

Quick&Dirty StateMachine from a year ago (C++) (Not long after I had started learning C++)
StateMachine.h
 

#ifndef pyH_StateMachine
#define pyH_StateMachine
#pragma once

#include "stdafx.h"
#include <memory>
#include <map>
#include <stack>
#include <set>
#include "StateMachineEvent.h"
#include "EventHandler.h"
#include "easylogging++.h"

class GameState;

typedef std::shared_ptr<GameState> GameStatePtr;

class StateMachine {

public:

	StateMachine(EventHandler* eh);

	/*
		Destructor also destroys all states.
	*/
	~StateMachine();

	/*
		Adds a state to the state-list, and generates an ID for it.
		@return int ID - The generated ID of the state
	*/
	int addState(GameStatePtr g);

	/*
		Removes a state from the state-list. IDs WILL NOT be re-indexed, in case that they were stored somewhere else.
	*/
	void removeState(int ID);

	/*
		Fetches the state for the given ID. Returns null-pointer if no state was found.
		@return std:shared_ptr<GameState> - The state with the given ID.
	*/
	GameStatePtr getState(int ID);

	/*
		Pushes the state found for the given ID on top of the active-states, and focuses it (All other active states will no longer be updated, but still be rendered.)
	*/
	void focusState(int ID);

	/*
		Pushes the state found for the given ID at the back of the active-states, and focuses the one at the top. (All other active states will no longer be updated, but still be rendered.)
	*/
	void unfocusState(int ID);

	/*
		Renders a state inactive, thus no longer rendering and updating it.
	*/
	void makeInactive(int ID);

	/*
		Updates the focused state.
	*/
	void update(const float delta);

	/*
		Renders all active states, from bottom to top.
	*/
	void render(const float interpolation);


private:
	std::map<int, GameStatePtr> gameStates;
	
	std::deque<int> activeStateIDs;
	std::vector<int> inactiveStateIDs;

	EventHandler* eh;
};
#endif

StateMachine.cpp
 

#include "StateMachine.h"
#include "GameState.h"

#include <iostream>

StateMachine::StateMachine(EventHandler* eh) : eh(eh) {
	//TODO LOG creation
}

StateMachine::~StateMachine(){
	
	LOG(INFO) << "<StateMachine> Destroyed.";

	activeStateIDs.clear();
	inactiveStateIDs.clear();
	for(auto pair : gameStates){
		pair.second->exit();
		pair.second.reset();
	}
	gameStates.clear();
}

int StateMachine::addState(GameStatePtr state){

	LOG(INFO) << "<StateMachine> State \"" << state->getName() << "\" added.";

	int id = gameStates.size();
	gameStates[id] = state;
	
	eh->triggerEvent<StateMachineEvent>(EventType::StateMachineEvent_Added, 1, id);

	return id;
}

void StateMachine::removeState(int ID){

	LOG(INFO) << "<StateMachine> State \"" << gameStates.at(ID)->getName() << "\" removed.";

	VEC_DEL(activeStateIDs, ID);
	VEC_DEL(inactiveStateIDs, ID);

	gameStates.at(ID)->exit();
	gameStates.at(ID).reset();
	gameStates.erase(ID);

	eh->triggerEvent<StateMachineEvent>(EventType::StateMachineEvent_Removed, 1, ID);
}

GameStatePtr StateMachine::getState(int ID){
	return gameStates.at(ID);
}

void StateMachine::focusState(int ID){
	if(!activeStateIDs.empty() && activeStateIDs.back() == ID) return;
	VEC_DEL(inactiveStateIDs, ID);
	VEC_DEL(activeStateIDs, ID);
	if(gameStates.count(ID)){
		activeStateIDs.push_front(ID);
	}

	eh->triggerEvent<StateMachineEvent>(EventType::StateMachineEvent_Focused, 1, ID);
}

void StateMachine::unfocusState(int ID){
	if(activeStateIDs.front() == ID) return;
	VEC_DEL(inactiveStateIDs, ID);
	VEC_DEL(activeStateIDs, ID);
	if(gameStates.count(ID)){
		activeStateIDs.push_back(ID);
	}

	eh->triggerEvent<StateMachineEvent>(EventType::StateMachineEvent_Unfocused, 1, ID);
}

void StateMachine::makeInactive(int ID){
	if(VEC_HAS(inactiveStateIDs, ID)) return;
	VEC_DEL(activeStateIDs, ID);
	inactiveStateIDs.push_back(ID);

	eh->triggerEvent<StateMachineEvent>(EventType::StateMachineEvent_MadeInactive, 1, ID);
}

void StateMachine::update(const float delta){

	if(!activeStateIDs.empty()) gameStates.at(activeStateIDs.front())->update(delta);
}

void StateMachine::render(const float interpolation){
	auto iter = activeStateIDs.rbegin();
	while(iter != activeStateIDs.rend()){
		gameStates.at(*iter)->render(interpolation);
		iter++;
	}
}

C++ EventHandler for the same game (That never got anywhere, by the way)

#ifndef pyH_EventHandler
#define pyH_EventHandler
#pragma once
#include "Event.h"
#include "EventType.h"
#include "GameEvent.h"
#include "SFML\Graphics.hpp"
#include <map>
#include <deque>
#include "IPoolable.h"
#include "stdafx.h"

class EventHandler {

public:
	void triggerEvent(EventType type, GameEvent* e);
	void triggerEvent(sf::Event::EventType type, sf::Event e);
	
	template<class T> void triggerEvent(EventType type, int n_args, ...);
	
	EventHandler& tmpConstraints(const std::initializer_list<int>& il);

	void hookInto(sf::Event::EventType type, fastdelegate::FastDelegate1<sf::Event, void> fun);
	void hookInto(EventType type, fastdelegate::FastDelegate1<GameEvent*, void> fun);

	~EventHandler();
	EventHandler();

	void recycleEvent(EventType type, GameEvent* e);
	template<class T> GameEvent* newEvent(EventType type, int n_args, ...);

private:
	std::map<sf::Event::EventType, A3D::AEvent<FastDelegate<void (sf::Event)>>> inputEvents;
	std::map<EventType, A3D::AEvent<FastDelegate<void(GameEvent*)>>> gameEvents;

	std::map<EventType, std::deque<GameEvent*>> eventPool;

	template<class T> GameEvent* newEvent(EventType type, va_list l);

	std::vector<int> tempConstraints;

	friend class GameEvent;
};
#endif
#include "EventHandler.h"
#include <iostream>

//HACK
#include "Events.h"
//!HACK

EventHandler& EventHandler::tmpConstraints(const std::initializer_list<int>& il){
	tempConstraints.clear();
	for(const auto& i : il){
		tempConstraints.push_back(i);
	}
	return *this;
}


void EventHandler::triggerEvent(EventType type, GameEvent* e){
	if (gameEvents.count(type) > 0){
		for (A3D::HDELEGATE hDG = gameEvents.at(type).GetDelegateList().begin(); hDG != gameEvents.at(type).GetDelegateList().end();){
			A3D::HDELEGATE hCurDG = hDG++; 
			
			VEC1_CONTAINS_VEC2(hCurDG->second, tempConstraints, vecContains);

			if (hCurDG->second.size() == 0 || vecContains) gameEvents.at(type).GetDelegate(hCurDG)(e);
		}
	}
	tempConstraints.clear();
}
void EventHandler::triggerEvent(sf::Event::EventType type, sf::Event e){
	if(inputEvents.count(type) > 0) TRIGGER_EVENT(inputEvents.at(type), e);
}

template<class T> void EventHandler::triggerEvent(EventType type, int n_args, ...){
	va_list args;
	va_start(args, n_args);
	this->triggerEvent(type, newEvent<T>(type, args));
	va_end(args);
}

void EventHandler::hookInto(sf::Event::EventType type, FastDelegate1<sf::Event, void> fun){
	if (inputEvents.count(type) == 0) inputEvents[type] =  A3D::AEvent<FastDelegate<void(sf::Event)>>();
	inputEvents.at(type) += fun;
}

void EventHandler::hookInto(EventType type, fastdelegate::FastDelegate1<GameEvent*, void> fun){
	if (gameEvents.count(type) == 0) gameEvents[type] = A3D::AEvent<FastDelegate<void(GameEvent*)>>();
	gameEvents.at(type).InsertDelegate(fun, tempConstraints);
}

void EventHandler::recycleEvent(EventType t, GameEvent* e){
	e->reset();
	if(eventPool.count(t) < 1) eventPool[t] = std::deque<GameEvent*>();
	eventPool.at(t).push_back(e);
}

template<class T> GameEvent* EventHandler::newEvent(EventType type, int n, ...){
	va_list args;
	va_start(args, n);
	GameEvent* gep = newEvent(type, args);
	va_end(args);
	return gep;
}

template<class T> GameEvent* EventHandler::newEvent(EventType type, va_list l){
	GameEvent* gep;
	if (eventPool.count(type) < 1) eventPool[type] = std::deque<GameEvent*>();
	if (eventPool.at(type).size() < 1)
		gep = static_cast<GameEvent*>(new T());
	else{
		gep = eventPool.at(type).front();
		eventPool.at(type).pop_front();
	}
	gep->createNew(l);
	return gep;
}

EventHandler::~EventHandler(){
	VEC_LOOP(eventPool, it){
		VEC_LOOP(it->second, it2){
			(*it2)->reset();
			delete (*it2);
		}
		it->second.clear();
	}
	eventPool.clear();

	gameEvents.clear();
	inputEvents.clear();
}

EventHandler::EventHandler(){
}

template void EventHandler::triggerEvent<CollisionEvent>(EventType type, int n_args, ...);
template void EventHandler::triggerEvent<StateMachineEvent>(EventType type, int n_args, ...);

It's the shortest C++ code of mine I found that could be read without that much context, but unfrotunately there are still a few things in there that would need clarification... EventType for example, or VEC_LOOP (just a helper method)

As far as I can remember, it did work, although I'm not sure of whether it had memory leaks or not.

Then there's some Java code, which is quite recent - I wanted to continue working on the game from back then, but didn't feel like trying to understand my own code again, so I just started a Java project instead. To have as much flexibility as possible, I tried introducing an entity system that would allow me to easily add different types of entities as a plugin later on.
Entity.java
 

public abstract class Entity {

	public abstract ParameterDescriptor[] getRequiredParameters();
	protected Body body;
	private int ID;
	public int ID(){
		return ID;
	}
	public Entity setID(int id){
		if(this.body != null) this.body.setUserData(id);
		this.ID = id;
		return this;
	}
	public Body getBody(){
		return body;
	}
	public boolean posInEntity(float x, float y){
		if(body == null) return false;
		for(Fixture f : this.body.getFixtureList()){
			if(f.testPoint(x, y)) return true;
		}
		return false;
	}
	public float getPositionX(){
		return this.body.getPosition().x;
	}
	public float getPositionY(){
		return this.body.getPosition().y;
	}
	public void setPosition(float x, float y){
		this.body.getPosition().set(x, y);
	}
	public abstract void kill(Runnable deadCallback);
	protected abstract Body createAndSpawnBody(World w);
	public void spawn(World w){
		this.body = createAndSpawnBody(w);
	}}

GameWorld.java
 

public class GameWorld {
	//Both of these together represent all "things" in the game world. No direct access is given: Instead, helper methods will add entities to both of these. TODO: Non-physical entities?
	private World world;
	private Array<Entity> entities;
	
	private int idCount = 0;
	
	private Deque<Entity> entitiesQueuedForRemoval;
	
	private HashMap<String, EntityFactory> entityFactories;
	
	//Application functions
	private Array<EntityFunction> functionsToBeApplied;
	
	public void update(float deltaTime){
		clearDeadEntities();
		world.step(deltaTime, 6, 2);
		
		for(Entity currentEntity : this.entities){
			
			//Applying functions
			for(Iterator<EntityFunction> it = this.functionsToBeApplied.iterator(); it.hasNext();)
				if(it.next().applyFunctionOn(currentEntity))
					it.remove();
		}
		
		//Function applying is done, clear array.
		this.functionsToBeApplied.clear();
		
	}
	
	/**
	 * Registers a new type of entity. Always overwrites old registrations, in case they exist.
	 * @param entityTypeName - Name that will be used to spawn an entity. Careful not to create conflicting names.
	 * @param entityFactory - Factory returning a new instance of that entity, with given parameters.
	 */
	public void registerEntity(String entityTypeName, EntityFactory entityFactory){
		if(entityTypeName == null || entityFactory == null) return;
		entityFactories.put(entityTypeName, entityFactory);
	}
	
	public boolean isRegistered(String entityTypeName){
		return entityFactories.keySet().contains(entityTypeName);
	}
	
	public int addEntity(String entityTypeName, EntityParameter params){
		Entity entity = entityFactories.get(entityTypeName).createEntity(params);
		entity.spawn(world);
		entities.add(entity.setID(nextID()));
		return entity.ID();
	}
	
	private int nextID(){
		return idCount++;
	}
	
	public void removeEntity(Entity e){
		if(entities.contains(e, false)) entitiesQueuedForRemoval.push(e);
	}
	
	private void clearDeadEntities(){
		for(Entity e : entitiesQueuedForRemoval){
			entities.removeValue(e, true);
			if(e.getBody() != null) world.destroyBody(e.getBody());
		}
		entitiesQueuedForRemoval.clear();
	}

	public void applyFunctionToAllEntitiesQueued(EntityFunction function){
		this.functionsToBeApplied.add(function);
	}
	public void applyFunctionToAllEntitiesInstantly(EntityFunction function)
	{
		for(Entity gameEntity : this.entities){
			if(function.applyFunctionOn(gameEntity)) break;
		}
	}
	
	public GameWorld(){
		entities = new Array<Entity>();
		entitiesQueuedForRemoval = new ArrayDeque<Entity>();
		entityFactories = new HashMap<String, EntityFactory>();
		world = new World(new Vector2(0, -10), true);
		world.setContactListener(new Box2DListener());
		functionsToBeApplied = new Array<EntityFunction>();
		
	}
	
}

The "apply function stuff" can be used like that in the update method of the game:
 

public void update(final float dt)
	{
		this.vector3 = levelCam.screenPosToWorld(Gdx.input.getX(), Gdx.input.getY(), this.vector3);
		this.hovered = null;
		
		this.world.applyFunctionToAllEntitiesQueued(new EntityFunction() {
			public boolean applyFunctionOn(Entity foundEntity) {
				if(foundEntity.posInEntity(vector3.x, vector3.y)){
					hovered = foundEntity;
					return true;
				}
				return false;
			}
		});
		
		this.world.applyFunctionToAllEntitiesQueued(new EntityFunction(){
			public boolean applyFunctionOn(Entity foundEntity){
				controllers.get(foundEntity.getClass()).getController().updateEntity(dt, foundEntity);
				return false;
			}
		});
		
		this.world.update(dt);
	}

I also have an eventhandler in this java version of the game, and a plugin loader to actually load the entity plugins and then fill the world with entities from a level file, but it'll be too much code if I put everything here.

 

I know that there's way too much code on one hand and not enough code to understand the code I posted on the other hand, but it'd be absolutely awesome to be able to get some suggestions! (Or suggestions about how I should post the code here without having to post too much)

Thank you so much.

Share this post


Link to post
Share on other sites

Some quick thoughts on your C++ (there's too much here in total for me to want to review everything):

  • some of those macros look nasty. (If they're not macros, don't use all-caps) I don't like seeing 'cute' code.
  • if-statements all on one line are readability hell. Also, always put braces around your if blocks, whether you need them or not.
  • variable argument handling can probably be replaced by templates, and become safer
  • not much point using at() if you're not using exceptions. May as well stick to normal [ ] syntax if you know the object is there, or find() if you don't.
Edited by Kylotan

Share this post


Link to post
Share on other sites
there's too much here in total for me to want to review everything

Yeah, I know, and I'm really sorry about that! I just had no clue how I could get some judgement on my current ability and what/how I could improve without posting actual code, so I just chose to post a few of the shorter examples I had. If you have a better idea, I'd gladly hear it :)!

 

 

some of those macros look nasty. (If they're not macros, don't use all-caps) I don't like seeing 'cute' if-statements all on one line are readability hell. Also, always put braces around your if blocks, whether you need them or not. variable argument handling can probably be replaced by templates, and become safer not much point using at() if you're not using exceptions. May as well stick to normal [ ] syntax if you know the object is there, or find() if you don't.

Thank you so much! Yeah, the all-caps vector thingies are macros. It was some time ago, but I remember having been really tired of having to write down the iterator-stuff all the time, so I just put them in a macro. What exactly speaks against them, though? As long as the employed method is known, it should be fine, since macro's only are text replacement, right?
The rest makes perfect sense, thanks!

By the way, might there have been by any chance something "general" you noticed that may indicate my "level" (And thus also what I need to do in order to improve)?

Oh well, in any case, thanks!

 

(More judgements are also always welcome :P )

Edited by Flyverse

Share this post


Link to post
Share on other sites

As long as the employed method is known, it should be fine, since macro's only are text replacement, right?

Macros are problematic because they are just dumb text substitution. This can lead to all sorts of fun and exciting bugs such as double-evaluating parameters causing unintended side-effects. They cannot be stepped into by the debugger, because the debugger's mapping of source to compiled instruction is based on what was compiled, not what you wrote. 

For the most part they offer only disadvantages. There are a handful of things you can only do with macros in C++, but they are either very contextually specific (leveraging __FILE__ and __LINE__ effectively, for example) or very advanced (preprocessor iteration) and should not be used for general cases. In most cases an inline function or a template function will serve you and others who need to work in your code better.

 

but I remember having been really tired of having to write down the iterator-stuff all the time

In modern C++ you can use range-for to avoid a lot of iterator boilerplate. In cases where range-for doesn't work, you can instead use auto to deduce the iterator type at compile time. Both options eliminate what is arguable the worst thing with iterators, which is their really long and repetitive type names.

Share this post


Link to post
Share on other sites
If you want to know what you should do differently in coding, the absolute best thing to do is what you avoided: figure out what you did in your previously written code.

If you're ever working on a large project, you'll have situations with difficult to understand code (yours or someone else's), and starting over will not be an option. You'll spend more time debugging than writing code. It would also be awesome if you had sufficient experience in what makes code difficult to understand so you'd just avoid doing that stuff, and do it in a more readable way. And it would be awesome if you are good enough at reading ugly code that you could be useful in tracking down bugs instead of that guy who always says it's someone else's fault even when it isn't, and doesn't matter in the first place.

Share this post


Link to post
Share on other sites

Mostly tl;dr (please consider GitHub), but just from skimming:

I think you may be better off replacing that std::map with your own container class that encapsulates a std::unordered_map and handles all the index manipulation you're doing. Alternatively, find a way to do this without significant indexes. I'm a little bit confused about why you have a map of states in the first place. I've seen state stacks, but this is the first time I've seen a state map.

 

Why is this thing happening?

	activeStateIDs.clear(); //what happens when std::deque destructs?
	inactiveStateIDs.clear(); //what happens when std::vector destructs?
	for(auto pair : gameStates){
		pair.second->exit(); //shouldn't GameState's destructor do this?
		pair.second.reset(); //what happens when a shared pointer destructs?
	}
	gameStates.clear(); //what happens w... well... you get the idea

Most significantly, why are you killing off the individual elements of that shared_ptr map? It seems to defeat the purpose of shared_ptr, which makes me think you may not be using shared_ptr correctly. Given that you're using auto instead of auto& there, I suspect that this may indeed be the case.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this