I find myself in the position of trying to consolidate some code, ending up with a function declared as follows:
int process_spell_target(int who, int what, int y0, int x0, int y1, int x1, int spell, int level, u32b flg,I'm not sure whether this is morally the right thing to do.
int region_id, int delay, int damage_div, bool one_grid, bool forreal, bool player,
void retarget(int *ty, int *tx, u32b *flg, int method, int level, bool full, bool *one_grid), bool *cancel)
Posts
14 comments:
Let us now take the next moment in silence, to ponder the merits, and demerits, of using a struct to hold this data instead, and just passing the struct...
...moments of silence don't answer very many coding questions for me.
Smell:
Long Parameter List
Common Refactorings:
Replace paramater with method, Introduce parameter object, Preserve whole object.
Although this book ("Refactoring" by Fowler) assumes that you are using an OOP language, but unangband is in C, no?
You should be at least able to
typedef void cb_target(int*, int*, u32b*, int, int, bool, bool*);
especially if you use such callbacks elsewhere.
It's that old programming truism in action: if your function starts with a dozen parameters, it will inevitably accrete a dozen more.
All: I'm far happier to leave a big hairy function declaration in place, as opposed to trying to hide this bundle of string inside something else.
I don't believe it's worthwhile creating an object, structure or other encapsulation, unless I'll actually get some re-use out of it. And I don't think I'll do so in this instance.
At least with a disgusting declaration like this, reading the code you know you're moving into uncharted waters. And it turns out that the code inside the function is actually relatively clean and straightforward to follow.
As for what this function does: It casts a spell from 'something' to a grid location. The complication comes from the fact that the spell could be create multiple effects, possibly requiring the target grid to be shifted each time, and the way the target is shifted depends on what 'something' is. If it's the player, we ask for user input, if it's a region, we pick another grid in the region, if it's a monster, we use some AI.
Then there's the cruft, which I'm slowly working through, which I originally had in the 3 places this code was used.
Another way of doing this would be to pass through an integer where there is a bijection between the integer and the number of spell effects to be cast, and increase this integer by one every iteration. But a significant chunk of the function computes how many effects occur, so I'll end up with duplicate code again. Hence the call back...
I don't claim to know the best answers, but in the spirit of constructive debate, I can think of just a few things off the top of my head:
- Typedef retarget()? Even if it's only used here, it would be a big readability boost: at a glance, the arguments to retarget() actually look like they're arguments to process_spell_target(). It would cut some confusion and a whole line...
- Pairs of coordinates as structs? Meh, in C this is probably not that helpful.
However, long lists of parameters are a good sign in the sense that you're probably not relying a lot on global information; which is good. I'm guilty of doing this in a project in MATLAB since the environment is messy already (you don't want globals there!) and every variable is copy-on-write so I'm in fact just passing references.
BTW my favorite parameter there: "bool forreal"! Is it for real? Yeah, fo' sure!!
I definitely think using a struct would be better, depending on what you're using the function for. In C++ I'd use a class.
i agree on the typedef for the func ptr, that's half the mess right there - otherwise i think the "real" refactoring to be done is reduce the amount that function actually does. i always like the rule of one function should do one thing roughly, and if there are giant branches inside of that function then it's probably worthwhile to split out variations (dosomething_player, dosomething_monster, etc). code duplication can still be minimized with inline helper functions of course....
If it is simpler for you, it is simpler. End of story.
I'd say it depends on the function's reuse. If it's only used in one or two places, it's fine if it's this long, and it's also likely since it probably does a lot. Other functions that are meant to be reused heavily will probably have a more limited scope, thus less parameters; and also enforcing their [reusable functions'] simplicity is good practice.
Count me as another voice for "this function is too big and complex." Ya know, functions can consolidate code used many times, and functions can isolate functionality to increase comprehension (and to "objectify" the functionality). I'm not sure your function does any of this.
- Instead of:
Funct(ungodly arguments and complexity)
- I'd do something like:
// *** Funct system starts
Data tempData;
// fill in data structure
FunctPart1(tempData);
if (FunctPart2(tempData))
FunctPart3(tempData);
// *** Funct system finished
Just my 2cents as a log-time C++ coder.
I have a nagging suspicion that this a case of one method which should have pieces in different classes.
If you would separate those parameters according to which are targeting data, which are spell identifiers and which are circumstance info, etc. maybe we/you could make some sense of it.
"- Pairs of coordinates as structs? Meh, in C this is probably not that helpful."
Not nearly as helpful in C as in C++. Also, the dungeon-generation code is a bit hypersensitive (making it difficult to do a full conversion of a *band to coordinate pair structs; Zaiband isn't fully converted).
Function typedef is needed.
I'd eliminate the bool parameters, simplify the resulting forked functions, and any remaining centralizable code fragments would be static functions.
Post a Comment