C++ best practices for current ToME2 code?

[Lord Estraven]

The below is an example of type of structure common in T2 code, in areas ported directly from the original C. This one's from traps.cc:

Code: Select all

/*
 * this function activates one trap type, and returns
 * a bool_ indicating if this trap is now identified
 */
bool_ player_activate_trap_type(s16b y, s16b x, object_type *i_ptr, s16b item)
{
	bool_ ident = FALSE;
	s16b trap;

	s16b k, l;

	trap = cave[y][x].t_idx;

	if (i_ptr != NULL)
	{
		trap = i_ptr->pval;
	}

	switch (trap)
	{
		/* stat traps */
	case TRAP_OF_WEAKNESS_I:
		ident = do_dec_stat(A_STR, STAT_DEC_TEMPORARY);
		break;
	case TRAP_OF_WEAKNESS_II:
		ident = do_dec_stat(A_STR, STAT_DEC_NORMAL);
		break;
	case TRAP_OF_WEAKNESS_III:
		ident = do_dec_stat(A_STR, STAT_DEC_PERMANENT);
		break;
	case TRAP_OF_INTELLIGENCE_I:
		ident = do_dec_stat(A_INT, STAT_DEC_TEMPORARY);
		break;
/*** Continues this way for a long while ***/

That's the beginning of the function. It continues for about a thousand lines, with the switch() cases getting progressively longer and more complex as it goes on. High cognitive overhead, much repeated code, etc.

The obvious answer in C is, split this up into smaller functions. And eventually, try to pass as much stuff as possible by function parameters, and get rid of those big switch() statements. Or so I'd think, anyway. But at the very least, the giant switch() could be split up into a bunch of smaller switch()ing functions with the trap type as a parameter.

(That'd be hackish, but would be better than the original for now.)

The problem is, this is supposed to be C++, not C. I have no idea what the standard answer to this is in typical C++98 paradigms, let alone functional-style C++11.

How should I be refactoring the code for stuff like this?

[AnonymousHero]

Right, so the key here is to identify whether all the branches of each of the cases have multiple "return" values (outputs). In this case, it looks like they do not -- they all seem to return/modify the local "ident" variable.

So, you can refactor each case into a function which takes a "bool *" (or whatever ident is) and replace that long list of cases with somewhat shorter

Code: Select all

    switch (condition_var) {
         case FOO: foo(&ident); break;
         case BAR: bar(&ident); break;
         ...
    }

(if none of the branches actually uses the initial value of "ident", you can just do a simple return value.)

If there are multiple "outputs" from each branch then you'll want to use "boost::tuple" and the tie(...) function, so that would be:

Code: Select all

    switch (condition_var) {
       case FOO: tie(ident, cost) = foo(); break;
    }

If you want to avoid the switch you can do that too if you want, by using e.g.

Code: Select all

    map<int, std::function(...)> callbacks;
    callbacks[condition_var](...);

(use proper parameter lists for ...)

However, for the given code: I'm not sure this would actually help that much since each branch happily modifies various globals, etc. I guess it might be a way to start reducing the amount of global access since you can give each of the foo()/bar()/... the thing they access globally as parameters instead, but...

(Plus there's the whole traps-must-go thing )

In general refactoring for maintainability is a huge area. I can recommend the book Working With Legacy Code by Feathers, though it's C++ advice is somewhat dated.

[Lord Estraven]

Thanks @AnonymousHero...

Hmm. I have to admit that the passing-mutable-bool-pointers thing seems weird to me. It sounds like an easy way to run into variable scoping issues. Also, void functions with mutable parameters and side effects just seems... Kind of oddball? It's less cluttered than using a bool return value and an assignment, but something just looks wrong about it.

Though, I don't think anything does use the value of ident. I believe that's just returned if the trap is to be identified.

Re boost::tuple and tie(): now that seems more like it! Seems like a bunch of stuff could be moved out of global/static/side effect territory, by putting it in tuple return values.

(Immediate questions on that:
1. Is there a way to do that without Boost, using only C++11 standard libraries?
2. For future reference, is there a sane way to have "tuples" in pure C? Because it seems really, really handy. Best I can come up with seems to be returning a static array. But that can be dangerous in C, with out-of-bounds accesses not being caught by the compiler...)

Re map(), that sounds fantastic but the syntax confuses me. I'll see if I can find online reference stuff about that.

...

For the code itself: yes, that's exactly what I was thinking. Reduce the amount of static/global clutter, and divvy up different stuff into different functions.

Traps were just a handy example though. Also, too bad they seem to be more heavily incorporated into the Theme module...

...

And thank you for the book recommendation. There are a bunch of legacy Linux apps that I've wanted to revitalize, so that sounds worthwhile.

[AnonymousHero]

Thanks @AnonymousHero...

Hmm. I have to admit that the passing-mutable-bool-pointers thing seems weird to me. It sounds like an easy way to run into variable scoping issues. Also, void functions with mutable parameters and side effects just seems... Kind of oddball? It's less cluttered than using a bool return value and an assignment, but something just looks wrong about it.

Re: variable scoping issues: Nah, lexical scope is preferred for a reason.

This case, is specifically for the case where the function actually needs the value of "ident" from the "outside" (i.e. before the switch statement).

Thanks @AnonymousHero...
Re boost::tuple and tie(): now that seems more like it! Seems like a bunch of stuff could be moved out of global/static/side effect territory, by putting it in tuple return values.

(Immediate questions on that:
1. Is there a way to do that without Boost, using only C++11 standard libraries?
2. For future reference, is there a sane way to have "tuples" in pure C? Because it seems really, really handy. Best I can come up with seems to be returning a static array. But that can be dangerous in C, with out-of-bounds accesses not being caught by the compiler...)

1. Actually, I made a mistake. It's in C++11, though a bit inconvenient to return since initializer list synax for it was introduced in C++17. See http://en.cppreference.com/w/cpp/utility/tuple
2. No, not really, I think. The shortest you can do is an anonymous struct, I think.

Re map(), that sounds fantastic but the syntax confuses me. I'll see if I can find online reference stuff about that.

Try looking it up on cppreference.com -- it's a fantastic resource and they have (small) examples for most things.

[Lord Estraven]

Huh. Last I checked, anonymous structs weren't a thing in C. Not sure if that changed with C11 though.

Variable scoping issues: right, the compiler would catch this. N/M. In any case, I don't think I've yet seen a function written such that what you describe would be necessary...

In any case, which code besides the trap stuff do you think most needs refactoring? I've noticed really hefty duplication in the wand/rod/staff code, and more giant switch()ing functions for scrolls and potions... Not sure what else there is though. I'm thinking I will start attacking the scroll/potion stuff this evening.

[AnonymousHero]

Huh. Last I checked, anonymous structs weren't a thing in C. Not sure if that changed with C11 though.

It did, apparently. (At least you can have them in unions in C11.)

Variable scoping issues: right, the compiler would catch this. N/M. In any case, I don't think I've yet seen a function written such that what you describe would be necessary...

I think there were at least a few, but it can be very hard to notice given the sheer amount of code.

In any case, which code besides the trap stuff do you think most needs refactoring? I've noticed really hefty duplication in the wand/rod/staff code, and more giant switch()ing functions for scrolls and potions... Not sure what else there is though. I'm thinking I will start attacking the scroll/potion stuff this evening.

I think some of these bits of the code need a thorough rethink of how spells/effects/etc. should really be desribed at a high level -- and then be rewritten to "interpret" such a high-level description instead of hardcoding everything. Just as a small example, most spells can actually be described as some variant of the Thaumaturgy combinations, e.g. "ball damage (level*5)d10+5 radius (level/25)" or some such, but they're basically all hardcoded and even worse their descriptions are basically all a tiny variations on the code that actually does the effect. If there was a meta-level most of the duplicate code would disappear completely. That would remove (at a guess) several thousand lines. EDIT: And in fact, the code behind all the hardcoded spells works in pretty much this way. For example, I remember Furyband has a class that could deliver a "ball MELEE damage radius 3" spell and it would actually hit (as in: melee) every monster in that radius. So really, this should be pushed to edit files.

EDIT: For an example, look at https://github.com/angband/angband/blob ... elements.h and the other list-* files. (Though Angband is slightly easier since it doesn't have many of these really "out there" spells/effects.)

(This is complicated horribly by the fact that some of the spells are wildly out there compared to this.)

Don't get me wrong; reducing duplication and such probably helps, but in the end it's probably not going to suddenly make the code maintainable.

[Lord Estraven]

FWIW the melee ball spell uses the GF_ATTACK damage type. ToME has had that for ages.

And I was actually thinking of doing the serious meta-refactor... Though admittedly, with the help of C preprocessor hackery that I probably shouldn't be touching. Basically generating looooong lists of spell effects at build time.

[Edit: whoah, it looks like that's what V is doing in your link.]

e.g. something like

Code: Select all

/* Somewhere in the headers */
#define gen_potion_func(x, y) ((x))(int pval, int pval2) { return ((y))(pval, pval2); }
gen_potion_func(SV_POTION_FOO_BAR, effect_bless);
gen_potion_func(SV_POTION_FOO_BUZ, effect_bless);
gen_potion_func(SV_POTION_BUZ_BUZ, effect_confuse);
/* etc. */
...

/* Somewhere inside the potion quaffing function */
#define call_potion_func(x) (x)(pval, pval2)
call_potion_func(sval);

This is probably a tad half baked right now, though. And also, again, it's using C methodology. And leaning heavily on macros.

[AnonymousHero]

Yeah, I'm not a fan of the X-macro, as it's called. There doesn't seem to be any quite-as-compact way of doing it in C++, but that doesn't make me like it more. Template traits are quite similar, but seem quite a bit more verbose.

It screws up IDEs, it induces quite high coupling (though it's compile-time), etc.

[Castler]

Like AnonymousHero said, I'd do some sort of callback map.

Code: Select all

using std::bind;

// A trap handler takes y, x parameters and return an ident flag
typedef std::function<bool(int, int)> TrapCallback;

// A map to look up a trap type and return a handler for that type
const std::map<int, TrapCallback> traps {
  // Create function objects that call do_dec_stat with the appropriate parameters.
  // It's okay that it doesn't need the x, y parameters we're passing in.
  { TRAP_OF_WEAKNESS_I, bind(do_dec_stat, A_STR, STAT_DEC_TEMPORARY) },
  { TRAP_OF_WEAKNESS_II, bind(do_dec_stat, A_STR, STAT_DEC_NORMAL) },
  { TRAP_OF_WEAKNESS_III, bind(do_dec_stat, A_STR, STAT_DEC_PERMANENT) },

  // For more complicated traps, you can use lambdas
  { TRAP_OF_EARTHQUAKE, [](int y, int x) {
    msg_print("As you touch the trap, the ground starts to shake.");
    earthquake(y, x, 10);
    return true;
  }}
};

bool player_activate_trap_type(int y, int x, int trap)
{
  // Gets an iterator pointing to the requested trap
  auto handler = traps.find(trap);
  if (handler == trap.end()) {
    // Error: unknown trap
  } else {
    // Invoke the handler
    handler->second(y, x);
  }
}

The other, more aggressive option is to go full OO.

Code: Select all

class Trap {
public:
  virtual void PlayerActivate(int y, int x) = 0;
};

Then each new trap type is a subclass of Trap. (You'd probably still want some sort of map or vector to map TRAP_... IDs to their subclasses.)

The OO approach could be more useful for stuff like potions, since you could (for example) easily put a healing potion's drink effect, smash effect, and trap effect within the same HealingPotion class, and adding a new potion would involve adding a new subclass (instead of currently having to touch a few different parts of the code).

[Castler]

Yeah, I'm not a fan of the X-macro, as it's called. There doesn't seem to be any quite-as-compact way of doing it in C++, but that doesn't make me like it more. Template traits are quite similar, but seem quite a bit more verbose.

I'd seen the technique before but didn't know of an official name for it. Thanks for the pointer.

Unfortunately, I kind of like X-macros. I know that "macros are evil" and I "should feel a vague sense of shame after using them," but there are times such as this where I consider them to be the cleanest solution to a problem. (Although I know there still good rationales against, so I wouldn't debate anyone who thinks otherwise.)

[Lord Estraven]

@Castler

Callback map, hmm. I was experimenting a bit with something like that for potions, though in the C style - just an array of function pointers, and calling the one corresponding to the potion SVAL.

I do sometimes wish T2 were still pure C. I know C++ has a lot of advantages, but simplicity is not one of them.

For OOP, I'd considered that too, but wasn't sure how/if it fit with the C++11 stuff. Also it would require bottom-up redesign. Though that might be a good thing... hmm. And I do like the idea of using fewer libraries and whatnot (both Boost and standard libraries). Since classes are a C++ builtin feature, etc.

I'll admit I kind of balk at hierarchical inheritance though. Not for any theoretical reasons; more just that it's been my programming bugbear since I learned Java back in college.

(And as far as macros being evil: maybe, but they beat manually duplicated code any day IMO.)