Cataclysm-dda: Fixing the "cram every functionality into one of 4 classes" anti-pattern

[Coolthulhu]

Many classes are bloated with functionality that could be rewritten using only public methods from said classes.

game, player, map, item - all of those are bloated with functions that interact with multiple unrelated objects at a time, arbitrarily picking one of them as the current this, then using mostly or only public methods.
For example:

• Finding ammo to reload is functionality of both Character and item, with no clear hierarchy
• map::collapse_at implements a whole behavior of collapsing, mostly using only pretty muc only public methods of the class, interacting with game (collapsing on critters)
• map::create_spores - it creates and infects creatures
• The entire projectile shooting mechanic is a Creature method, but could easily be implemented with just a Creature argument

This looks very much like an anti-pattern, something to avoid. There is no clear responsibility, no encapsulation, bloated headers, multi-thousand line files, no... pattern.

Is there an obvious course of action? Do we have a plan here?

I'd do it like this:

• When implementing new function that doesn't have a very clear "owner", try to find a neutral ground for it, by writing it as a class-less function inside a namespace
• Groups of functions, such as most functions related to consumption, should likewise be moved to a namespace. Alternatively, a wrapper class or a similar class as player_morale

Is there a good, short guideline that could be a good starting point for designing class interactions? Something that could be linked in PRs with just "it should look like this: [link], you can move fun_x and fun_y outside the class because it uses only public methods".

EDIT: It looks like https://en.wikipedia.org/wiki/Facade_pattern and https://en.wikipedia.org/wiki/Decorator_pattern could be useful here

[kevingranade]

There's no silver bullet for this kind of thing. There are a few things we
can do project wide to encourage moving in the right direction, but they
just exert pressure, they don't provide solutions.

The simplest is to enfirce a rule that the larger classes (some arbitrary
size, 1,000+ lines or so) simply cannot be made longer. In other words, if
you want to add functionality to a class you have to cut something else
first. This can apply to header size, definition size, number of public
methods, etc. This WILL keep otherwise good code from landing, I guarantee
it.

Other than that you just need to chop up some classes. The main thing you
need to do is identify what should be in the big classes so you have a
good way to decide what should stay and what should go. For example the
game class should be the top level container for the active game instance,
but it shouldn't also own the main game loop do_turn(), that should go
elsewhere, as should all the ui code that's jammed in there. The player
class should own code that changes the state of the player, but ui code and
code implementing common functionality like ranged.cpp should be factored
out.

As these smaller classes are extracted, their responsibilities can be
outlined with unit tests, the lack of which bothers me a lot more than
class sizes (though they do influence each other).

The most successful example if this so far has been the character class, it
extracted quite a lot of common code from player and npc.

Another good thing to do would be to more fully isolate some of the
classes. item, map, vehicle and creature are good candidates for evolving
into the sole interfaces for their respective implementations, ideally each
of these would normally be accessed via the interface classes, with the
only exceptions being json loading and similar background tasks

[Zireael07]

I guess you could enforce a 'no new features' block before 0.D and spend some time cleaning those up before actually shipping 0.D?

[Coolthulhu]

item, map, vehicle and creature are good candidates for evolving into the sole interfaces for their respective implementations, ideally each of these would normally be accessed via the interface classes, with the only exceptions being json loading and similar background tasks

What do you mean by "interface" here? An inherited class that does the job? Something like visitable template?

[CoroNaut]

I really like Zireael07's idea to repair damaged code. This would give everyone much more time to discuss what features should be kept in given mods and balancing problems. It would give time to organize the code for performance and ease of access purposes. Finally, it would focus efforts on the new release to make sure that there are no major bugs that would steer the community away. We could have a grand opening and prepare an accurate list on all the new features. :D

[kevingranade]

On Feb 7, 2017 6:10 AM, "Coolthulhu" [e-mail] wrote:

item, map, vehicle and creature are good candidates for evolving into the
sole interfaces for their respective implementations, ideally each of these
would normally be accessed via the interface classes, with the only
exceptions being json loading and similar background tasks

What do you mean by "interface" here? An inherited class that does the job?
Something like visitable template?

My bad, I shouldn't have used such an overloaded term. I mean a class that
is visible to the rest of the program. For example the item class would be
visible and usable everywhere, but itype should only be called by item and
friends. Applied consistently, this would mean fewer changes would require
edits spanning the entire codebase and generally easier to understand code.

[kevingranade]

On Feb 7, 2017 12:01 AM, "Zireael07" [e-mail] wrote:

I guess you could enforce a 'no new features' block before 0.D and spend
some time cleaning those up before actually shipping 0.D?

This would need to be after 0.D, this kind of cleanup is good for stability
and maintainability long term, but it's bad for stability while in
progress.

[Zireael07]

@kevingranade how far away are we from 0.D? I agree that you have a point about the cleanup being after the first stable release in a long time, but I wouldn't want this issue to get forgotten...

[codemime]

I can't help but noticing that a considerable part of the player class consists of something like:

bool can_do_stuff(...);
void do_stuff(...);

These functions could be extracted into separate classes (somewhat similar to iuse_actor) which would accept player (or ever Creature) as an argument.

[kevingranade]

Completed discussion, the issue isn't meaningfully tracking progress, so closing.