Jump to content

  • Log In with Google      Sign In   
  • Create Account

#ActualHodgman

Posted 06 May 2013 - 01:08 AM


Number 2 isn't ugly; it makes the dependencies between your classes explicit and obvious.
If when constructing a ModelLoader, it requires a DataCache as an argument, then it's clear that this dependency exists, and it's easy to follow the layering of the different parts of your architecture.
On the other hand, if the ModelLoader internally acquires a DataCache via a global/singleton, then this is a magical, hidden dependency. This obfuscates the architecture, makes initialization ordering non-obvious, and makes the code much more brittle (harder to maintain) in the long run, due to all the hidden interactions.

 
Just playing devils advocate here, but couldn't you make the argument that that's a good thing? i.e. I need a to load a model, so I create a ModelLoader; I don't actually care how it internally loads models. And if someone one day decides to change how ModelLoader loads models, I don't want my code to suddenly break.


Yes, but no wink.png
 
There's 3 designs below; what you're describing could be implemented as #1 or #2. I was discouraging #1 and recommending #3.
If there is a 1:1 relationship between the loader and the cache, then #2 is ok. If multiple loaders can share the cache, then you should use #3.
#3 also follows inversion of control, a principle that generally makes code more flexible, testable and maintainable in my experience -- making components that are explicitly plugged together by the user allows them to be used in many more ways that components that automatically create/link their own inter-object dependencies.
#1 is the ugliest and most fragile IMO, so should almost never be used. It doesn't do what you're advocating -- removing any care of the internals from the user of the class -- it only seems like it does on a superficial level. In this case, the user still needs to ensure that the global caching system has been initialized before creating their ModelLoader1, and must ensure that all their ModelLoader1's have been destroyed before the global cache is destroyed.
struct FileSystemLocation {};
struct Name{};
struct Asset{ virtual ~Asset(){} };
struct Model : Asset{};
struct DataCache
{
	DataCache( const FileSystemLocation& );
	Asset* Load( const Name& );
};
class ModelLoader1
{
public:
	Model* Load( const Name& name )
	{
		extern DataCache* g_Cache;
		assert( g_Cache );
		return dynamic_cast<Model*>( g_Cache->Load( name ) );
	}
};

class ModelLoader2
{
private:
	DataCache m_cache;
public:
	ModelLoader2( const FileSystemLocation& fs ) : m_cache(fs) {}
	Model* Load( const Name& name )
	{
		return dynamic_cast<Model*>( m_cache.Load( name ) );
	}
};

class ModelLoader3
{
private:
	DataCache& m_cache;
public:
	ModelLoader3( DataCache& c ) : m_cache(c) {}
	Model* Load( const Name& name )
	{
		return dynamic_cast<Model*>( m_cache.Load( name ) );
	}
};
DataCache* g_Cache = 0;
void test1()
{
	{
		FileSystemLocation fs = {};
		DataCache globalCache( fs );
		g_Cache = &globalCache;
		ModelLoader1 loader;
		Model* model = loader.Load( Name() );
	}
	g_Cache = 0;
}
int test2()
{
	FileSystemLocation fs = {};
	ModelLoader2 loader( fs );
	Model* model = loader.Load( Name() );
}
int test3()
{
	FileSystemLocation fs = {};
	DataCache cache( fs );
	ModelLoader3 loader( cache );
	Model* model = loader.Load( Name() );
}
As for your code breaking if the design of the loader is changed... with #2 and #3, you'll get compile time errors if this occurs. With #1 your code might still compile but then crash because the hidden global dependencies might not be correctly configured for the new design. The fact that your code breaks at compile time when design incompatibilities are introduced is IMO a very good thing™.

Especially for my game states, since every game state might create another game state that needs some other of those "managers", I basically have to pass and store them into each one of them. Are there any good strategies to handle this sort of (growing) complexity? Talking generally for situations where I have to pass many references around that are not directly related, but could theoretically be grouped...

The 'context pattern' is often used to address this. If all of your game states requires that long list of arguments, then you can put them into a context structure:
struct GfxResourceContext
{
	gfx::Resources<Texture>& textures;
	gfx::Resources<Material>& materials;
	gfx::Resources<Mesh>& meshes;
	gfx::Resources<Effect>& effects;
};		  
struct EcsManagerContext
{
	ecs::EntityManager& entity;
	ecs::SystemManager& system;
	ecs::MessageManager& message;
};		  
struct GameStateContext
{
	GfxResourceContext resources;
	EcsManagerContext managers;
};
...
MainMenuState( GameStateContext& ctx );

#8Hodgman

Posted 06 May 2013 - 01:07 AM


Number 2 isn't ugly; it makes the dependencies between your classes explicit and obvious.
If when constructing a ModelLoader, it requires a DataCache as an argument, then it's clear that this dependency exists, and it's easy to follow the layering of the different parts of your architecture.
On the other hand, if the ModelLoader internally acquires a DataCache via a global/singleton, then this is a magical, hidden dependency. This obfuscates the architecture, makes initialization ordering non-obvious, and makes the code much more brittle (harder to maintain) in the long run, due to all the hidden interactions.

 
Just playing devils advocate here, but couldn't you make the argument that that's a good thing? i.e. I need a to load a model, so I create a ModelLoader; I don't actually care how it internally loads models. And if someone one day decides to change how ModelLoader loads models, I don't want my code to suddenly break.


Yes, but no wink.png
 
There's 3 designs below; what you're describing could be implemented as #1 or #2. I was discouraging #1 and recommending #3.
If there is a 1:1 relationship between the loader and the cache, then #2 is ok. If multiple loaders can share the cache, then you should use #3.
#3 also follows inversion of control, a principle that generally makes code more flexible, testable and maintainable in my experience -- making components that are explicitly plugged together by the user allows them to be used in many more ways that components that automatically create/link their own inter-object dependencies.
#1 is the ugliest and most fragile IMO, so should almost never be used. It doesn't do what you're advocating -- removing any care of the internals from the user of the class -- it only seems like it does on a superficial level. In this case, the user still needs to ensure that the global caching system has been initialized before creating their ModelLoader1, and must ensure that all their ModelLoader1's have been destroyed before the global cache is destroyed.
struct FileSystemLocation {};
struct Name{};
struct Asset{ virtual ~Asset(){} };
struct Model : Asset{};
struct DataCache
{
	DataCache( const FileSystemLocation& );
	Asset* Load( const Name& );
};
class ModelLoader1
{
public:
	Model* Load( const Name& name )
	{
		extern DataCache* g_Cache;
		assert( g_Cache );
		return dynamic_cast<Model*>( g_Cache->Load( name ) );
	}
};

class ModelLoader2
{
private:
	DataCache m_cache;
public:
	ModelLoader2( const FileSystemLocation& fs ) : m_cache(fs) {}
	Model* Load( const Name& name )
	{
		return dynamic_cast<Model*>( m_cache.Load( name ) );
	}
};

class ModelLoader3
{
private:
	DataCache& m_cache;
public:
	ModelLoader3( DataCache& c ) : m_cache(c) {}
	Model* Load( const Name& name )
	{
		return dynamic_cast<Model*>( m_cache.Load( name ) );
	}
};
DataCache* g_Cache = 0;
void test1()
{
	{
		FileSystemLocation fs = {};
		DataCache globalCache( fs );
		g_Cache = &globalCache;
		ModelLoader1 loader;
		Model* model = loader.Load( Name() );
	}
	g_Cache = 0;
}
int test2()
{
	FileSystemLocation fs = {};
	ModelLoader2 loader( fs );
	Model* model = loader.Load( Name() );
}
int test3()
{
	FileSystemLocation fs = {};
	DataCache cache( fs );
	ModelLoader3 loader( cache );
	Model* model = loader.Load( Name() );
}
As for your code breaking if the design of the loader is changed... with #2 and #3, you'll get compile time errors if this occurs. With #1 your code might still compile but then crash because the hidden global dependencies might not be correctly configured for the new design. The fact that your code breaks at compile time when design incompatibilities are introduced is IMO a very good thing™.

Especially for my game states, since every game state might create another game state that needs some other of those "managers", I basically have to pass and store them into each one of them. Are there any good strategies to handle this sort of (growing) complexity? Talking generally for situations where I have to pass many references around that are not directly related, but could theoretically be grouped...

The 'context pattern' is often used to address this. If all of your game states requires that long list of arguments, then you can put them into a context structure:
struct ResourceContext
{
	gfx::Resources<Texture>& textures;
	gfx::Resources<Material>& materials;
	gfx::Resources<Mesh>& meshes;
	gfx::Resources<Effect>& effects;
};		  
struct ManagerContext
{
	ecs::EntityManager& entity;
	ecs::SystemManager& system;
	ecs::MessageManager& message;
};		  
struct GameStateContext
{
	ResourceContext resources;
	ManagerContext managers;
};
...
MainMenuState( GameStateContext& ctx );

#7Hodgman

Posted 06 May 2013 - 01:05 AM


Number 2 isn't ugly; it makes the dependencies between your classes explicit and obvious.
If when constructing a ModelLoader, it requires a DataCache as an argument, then it's clear that this dependency exists, and it's easy to follow the layering of the different parts of your architecture.
On the other hand, if the ModelLoader internally acquires a DataCache via a global/singleton, then this is a magical, hidden dependency. This obfuscates the architecture, makes initialization ordering non-obvious, and makes the code much more brittle (harder to maintain) in the long run, due to all the hidden interactions.

 
Just playing devils advocate here, but couldn't you make the argument that that's a good thing? i.e. I need a to load a model, so I create a ModelLoader; I don't actually care how it internally loads models. And if someone one day decides to change how ModelLoader loads models, I don't want my code to suddenly break.


Yes, but no wink.png
 
There's 3 designs below; what you're describing could be implemented as #1 or #2. I was discouraging #1 and recommending #3.
If there is a 1:1 relationship between the loader and the cache, then #2 is ok. If multiple loaders can share the cache, then you should use #3.
#3 also follows inversion of control, a principle that generally makes code more flexible, testable and maintainable in my experience -- making components that are explicitly plugged together by the user allows them to be used in many more ways that components that automatically create/link their own inter-object dependencies.
#1 is the ugliest and most fragile IMO, so should almost never be used. It doesn't do what you're advocating -- removing any care of the internals from the user of the class -- it only seems like it does on a superficial level. In this case, the user still needs to ensure that the global caching system has been initialized before creating their ModelLoader1, and must ensure that all their ModelLoader1's have been destroyed before the global cache is destroyed.
struct FileSystemLocation {};
struct Name{};
struct Asset{ virtual ~Asset(){} };
struct Model : Asset{};
struct DataCache
{
	DataCache( const FileSystemLocation& );
	Asset* Load( const Name& );
};
class ModelLoader1
{
public:
	Model* Load( const Name& name )
	{
		extern DataCache* g_Cache;
		assert( g_Cache );
		return dynamic_cast<Model*>( g_Cache->Load( name ) );
	}
};

class ModelLoader2
{
private:
	DataCache m_cache;
public:
	ModelLoader2( const FileSystemLocation& fs ) : m_cache(fs) {}
	Model* Load( const Name& name )
	{
		return dynamic_cast<Model*>( m_cache.Load( name ) );
	}
};

class ModelLoader3
{
private:
	DataCache& m_cache;
public:
	ModelLoader3( DataCache& c ) : m_cache(c) {}
	Model* Load( const Name& name )
	{
		return dynamic_cast<Model*>( m_cache.Load( name ) );
	}
};
DataCache* g_Cache = 0;
void test1()
{
	{
		FileSystemLocation fs = {};
		DataCache globalCache( fs );
		g_Cache = &globalCache;
		ModelLoader1 loader;
		Model* model = loader.Load( Name() );
	}
	g_Cache = 0;
}
int test2()
{
	FileSystemLocation fs = {};
	ModelLoader2 loader( fs );
	Model* model = loader.Load( Name() );
}
int test3()
{
	FileSystemLocation fs = {};
	DataCache cache( fs );
	ModelLoader3 loader( cache );
	Model* model = loader.Load( Name() );
}
As for your code breaking if the design of the loader is changed... with #2 and #3, you'll get compile time errors if this occurs. With #1 your code might still compile but then crash because the hidden global dependencies might not be correctly configured for the new design.

Especially for my game states, since every game state might create another game state that needs some other of those "managers", I basically have to pass and store them into each one of them. Are there any good strategies to handle this sort of (growing) complexity? Talking generally for situations where I have to pass many references around that are not directly related, but could theoretically be grouped...

The 'context pattern' is often used to address this. If all of your game states requires that long list of arguments, then you can put them into a context structure:
struct ResourceContext
{
	gfx::Resources<Texture>& textures;
	gfx::Resources<Material>& materials;
	gfx::Resources<Mesh>& meshes;
	gfx::Resources<Effect>& effects;
};		  
struct ManagerContext
{
	ecs::EntityManager& entity;
	ecs::SystemManager& system;
	ecs::MessageManager& message;
};		  
struct GameStateContext
{
	ResourceContext resources;
	ManagerContext managers;
};
...
MainMenuState( GameStateContext& ctx );

#6Hodgman

Posted 06 May 2013 - 01:00 AM


Number 2 isn't ugly; it makes the dependencies between your classes explicit and obvious.
If when constructing a ModelLoader, it requires a DataCache as an argument, then it's clear that this dependency exists, and it's easy to follow the layering of the different parts of your architecture.
On the other hand, if the ModelLoader internally acquires a DataCache via a global/singleton, then this is a magical, hidden dependency. This obfuscates the architecture, makes initialization ordering non-obvious, and makes the code much more brittle (harder to maintain) in the long run, due to all the hidden interactions.

 
Just playing devils advocate here, but couldn't you make the argument that that's a good thing? i.e. I need a to load a model, so I create a ModelLoader; I don't actually care how it internally loads models. And if someone one day decides to change how ModelLoader loads models, I don't want my code to suddenly break.


Yes, but no wink.png
 
There's 3 designs below; what you're describing could be implemented as #1 or #2. I was discouraging #1 and recommending #3.
If there is a 1:1 relationship between the loader and the cache, then #2 is ok. If multiple loaders can share the cache, then you should use #3.
#3 also follows inversion of control, a principle that generally makes code more flexible, testable and maintainable in my experience -- making components that are explicitly plugged together by the user allows them to be used in many more ways that components that automatically create/link their own inter-object dependencies.
#1 is the ugliest and most fragile IMO, so should almost never be used. It doesn't do what you're advocating -- removing any care of the internals from the user of the class -- it only seems like it does on a superficial level. In this case, the user still needs to ensure that the global caching system has been initialized before creating their ModelLoader1, and must ensure that all their ModelLoader1's have been destroyed before the global cache is destroyed.
struct FileSystemLocation {};
struct Name{};
struct Asset{ virtual ~Asset(){} };
struct Model : Asset{};
struct DataCache
{
	DataCache( const FileSystemLocation& );
	Asset* Load( const Name& );
};
class ModelLoader1
{
public:
	Model* Load( const Name& name )
	{
		extern DataCache* g_Cache;
		assert( g_Cache );
		return dynamic_cast<Model*>( g_Cache->Load( name ) );
	}
};

class ModelLoader2
{
private:
	DataCache m_cache;
public:
	ModelLoader2( const FileSystemLocation& fs ) : m_cache(fs) {}
	Model* Load( const Name& name )
	{
		return dynamic_cast<Model*>( m_cache.Load( name ) );
	}
};

class ModelLoader3
{
private:
	DataCache& m_cache;
public:
	ModelLoader3( DataCache& c ) : m_cache(c) {}
	Model* Load( const Name& name )
	{
		return dynamic_cast<Model*>( m_cache.Load( name ) );
	}
};
DataCache* g_Cache = 0;
void test1()
{
	{
		FileSystemLocation fs = {};
		DataCache globalCache( fs );
		g_Cache = &globalCache;
		ModelLoader1 loader;
		Model* model = loader.Load( Name() );
	}
	g_Cache = 0;
}
int test2()
{
	FileSystemLocation fs = {};
	ModelLoader2 loader( fs );
	Model* model = loader.Load( Name() );
}
int test3()
{
	FileSystemLocation fs = {};
	DataCache cache( fs );
	ModelLoader3 loader( cache );
	Model* model = loader.Load( Name() );
}
As for your code breaking if the design of the loader is changed... with #2 and #3, you'll get compile time errors if this occurs. With #1 your code might still compile but then crash because the hidden global dependencies might not be correctly configured for the new design.

#5Hodgman

Posted 06 May 2013 - 12:58 AM


Number 2 isn't ugly; it makes the dependencies between your classes explicit and obvious.
If when constructing a ModelLoader, it requires a DataCache as an argument, then it's clear that this dependency exists, and it's easy to follow the layering of the different parts of your architecture.
On the other hand, if the ModelLoader internally acquires a DataCache via a global/singleton, then this is a magical, hidden dependency. This obfuscates the architecture, makes initialization ordering non-obvious, and makes the code much more brittle (harder to maintain) in the long run, due to all the hidden interactions.

 
Just playing devils advocate here, but couldn't you make the argument that that's a good thing? i.e. I need a to load a model, so I create a ModelLoader; I don't actually care how it internally loads models. And if someone one day decides to change how ModelLoader loads models, I don't want my code to suddenly break.


Yes, but no wink.png
 
There's 3 designs below; what you're describing could be implemented as #1 or #2. I was discouraging #1 and recommending #3.
If there is a 1:1 relationship between the loader and the cache, then #2 is ok. If multiple loaders can share the cache, then you should use #3.
#3 also follows inversion of control, a principle that generally makes code more flexible, testable and maintainable in my experience -- making components that are explicitly plugged together by the user allows them to be used in many more ways that components that automatically create/link their own inter-object dependencies.
#1 is the ugliest and most fragile IMO, so should almost never be used. It doesn't do what you're advocating -- removing any care of the internals from the user of the class -- it only seems like it does on a superficial level. In this case, the user still needs to ensure that the global caching system has been initialized before creating their ModelLoader1, and must ensure that all their ModelLoader1's have been destroyed before the global cache is destroyed.
struct FileSystemLocation {};
struct Name{};
struct Asset{ virtual ~Asset(){} };
struct Model : Asset{};
struct DataCache
{
	DataCache( const FileSystemLocation& );
	Asset* Load( const Name& );
};
class ModelLoader1
{
public:
	Model* Load( const Name& name )
	{
		extern DataCache* g_Cache;
		assert( g_Cache );
		return dynamic_cast<Model*>( g_Cache->Load( name ) );
	}
};

class ModelLoader2
{
private:
	DataCache m_cache;
public:
	ModelLoader2( const FileSystemLocation& fs ) : m_cache(fs) {}
	Model* Load( const Name& name )
	{
		return dynamic_cast<Model*>( m_cache.Load( name ) );
	}
};

class ModelLoader3
{
private:
	DataCache& m_cache;
public:
	ModelLoader3( DataCache& c ) : m_cache(c) {}
	Model* Load( const Name& name )
	{
		return dynamic_cast<Model*>( m_cache.Load( name ) );
	}
};
DataCache* g_Cache = 0;
void test1()
{
	{
		FileSystemLocation fs = {};
		DataCache globalCache( fs );
		g_Cache = &globalCache;
		ModelLoader1 loader;
		Model* model = loader.Load( Name() );
	}
	g_Cache = 0;
}
int test2()
{
	FileSystemLocation fs = {};
	ModelLoader2 loader( fs );
	Model* model = loader.Load( Name() );
}
int test3()
{
	FileSystemLocation fs = {};
	DataCache cache( fs );
	ModelLoader3 loader( cache );
	Model* model = loader.Load( Name() );
}

#4Hodgman

Posted 06 May 2013 - 12:57 AM


Number 2 isn't ugly; it makes the dependencies between your classes explicit and obvious.
If when constructing a ModelLoader, it requires a DataCache as an argument, then it's clear that this dependency exists, and it's easy to follow the layering of the different parts of your architecture.
On the other hand, if the ModelLoader internally acquires a DataCache via a global/singleton, then this is a magical, hidden dependency. This obfuscates the architecture, makes initialization ordering non-obvious, and makes the code much more brittle (harder to maintain) in the long run, due to all the hidden interactions.

 
Just playing devils advocate here, but couldn't you make the argument that that's a good thing? i.e. I need a to load a model, so I create a ModelLoader; I don't actually care how it internally loads models. And if someone one day decides to change how ModelLoader loads models, I don't want my code to suddenly break.


Yes, but no wink.png
 
What you're describing could be implemented as #1 or #2 below. I was discouraging #1 and recommending #3.
If there is a 1:1 relationship between the loader and the cache, then #2 is ok. If multiple loaders can share the cache, then you should use #3.
#3 also follows inversion of control, a principle that generally makes code more flexible, testable and maintainable in my experience -- making components that are explicitly plugged together by the user allows them to be used in many more ways that components that automatically create/link their own inter-object dependencies.
#1 IMO should almost never be used. It doesn't do what you're advocating -- removing any care of the internals from the user of the class -- it only seems like it does on a superficial level. In this case, the user still needs to ensure that the global caching system has been initialized before creating their ModelLoader1, and must ensure that all their ModelLoader1's have been destroyed before the global cache is destroyed.
struct FileSystemLocation {};
struct Name{};
struct Asset{ virtual ~Asset(){} };
struct Model : Asset{};
struct DataCache
{
	DataCache( const FileSystemLocation& );
	Asset* Load( const Name& );
};
class ModelLoader1
{
public:
	Model* Load( const Name& name )
	{
		extern DataCache* g_Cache;
		assert( g_Cache );
		return dynamic_cast<Model*>( g_Cache->Load( name ) );
	}
};

class ModelLoader2
{
private:
	DataCache m_cache;
public:
	ModelLoader2( const FileSystemLocation& fs ) : m_cache(fs) {}
	Model* Load( const Name& name )
	{
		return dynamic_cast<Model*>( m_cache.Load( name ) );
	}
};

class ModelLoader3
{
private:
	DataCache& m_cache;
public:
	ModelLoader3( DataCache& c ) : m_cache(c) {}
	Model* Load( const Name& name )
	{
		return dynamic_cast<Model*>( m_cache.Load( name ) );
	}
};
DataCache* g_Cache = 0;
void test1()
{
	{
		FileSystemLocation fs = {};
		DataCache globalCache( fs );
		g_Cache = &globalCache;
		ModelLoader1 loader;
		Model* model = loader.Load( Name() );
	}
	g_Cache = 0;
}
int test2()
{
	FileSystemLocation fs = {};
	ModelLoader2 loader( fs );
	Model* model = loader.Load( Name() );
}
int test3()
{
	FileSystemLocation fs = {};
	DataCache cache( fs );
	ModelLoader3 loader( cache );
	Model* model = loader.Load( Name() );
}

PARTNERS