Sunday, July 23, 2017

About type design for layered architectures

Whenever you create a struct or class you should look at that class and ask yourself: "If I pass this type to a function, will that function or method affect or require EVERY member of this type?"

If not, then you're likely using a single type to group unrelated data used for different things. Sometimes it's unavoidable for very small types such as an array_view containing the members *Data and Count members, then size() will likely just access Count and data() will likely just access this->Data, but the more members your class have, the most likely you're grouping unrelated data together, causing confusion to everyone trying to understand your type, adding to code bloat, increasing writing/reading times, mouse wheel scrolling, build times, dependencies, etc. while decreasing readability, maintainability, reusability and so on. This clearly goes against every advice known by everyone such as KISS, DRY and YAGNI and others such as favoring composition over inheritance.

The solution to this is to group your members according a given task. i.e. instead of this:

struct Window { int posx, posy, sizex, sizey; int color; };

do this:


struct Coord { x, y; }; // you could even move this to a "coord.h" file for including it in files unrelated to "Window" 
struct Window { Coord pos, size; int color; };

Here is an example of poorly designed types taken from Doom 3 source code. You will quickly notice how there is a huge type with a large amount of unrelated members, with an unreadable amount of methods that have access to members they don't need and shouldn't have access to. Notice also that these types are meant to be inherited by other classes only to increase the problem.

https://github.com/id-Software/DOOM-3/blob/master/neo/game/Entity.h

Notice how the idEntity class have members like health put together with scriptObject, renderView, cinematic, flags, name along with the inherited members of idClass such as memused, numobjects, typegroup and also AddDamageEffect(), Save(), ClientReceiveEvent(), GetAnimator() and so on, all of which could easily be grouped into smaller separate systems that are focused on working on a whole abstract type instead of just a few members of it and enabling to use those smaller types in other classes unrelated to idEntity or idClass, really cutting down the amount of analysis required to read, write and maintain each method or type.

Notice also the inconsistency in the naming convention which sometimes is lowerCamelCase, sometimes is lowercase and it's really hard to find where the members come from because you have to look into different files of 500 lines each to look for the members and in particular class idEntity declares members spread along the whole file instead of grouping everything together.
Lastly, the comment that reads Animated entity base class. not only occupies 8 lines for clarifying something not requiring clarification, but also it's in the wrong place because it's not at the top of idAnimatedEntity.



No comments:

Post a Comment