How does DRY combine with "Separation of concern" and "One function, one task"?

Question

Possible Duplicate:
How many lines of code should a function/procedure/method have?

Out team has a project of not well structured ansi-c code. I would like to use some CC techniques to tidy up the code base.

As for C code, we have a lot of pointers and a lot of NULL-pointer pitfalls to catch. Therefore there are lot of fragments which look alike. It's like

if (pointer == NULL)
{
  function1();
  function2();
}

all over the place. Then there are an awful lot of functions that will be called after each other in the same fashion only with a few variations, like

function1();
function2a();
function3();

and

function1();
function2b();
function3();

all over the place.

I would like to extract those blocks as a single function to reduce LOC and copy-pasting. But that will create not only a (somewhat) orthogonal layer but also a handfull of function doing more or less the same, except for some details. And even worse, it will create functions that do a lot of things at once.

So, what's a good strategy? What's more important, lean code on high level, lean functions on low level or lean architecture? Which principle trumps the other? Seperation of concern or DRY?

I would like to refactor that beast but don't know where to start.

To exploid the example below and put same names in. Lets assume we have

morningBath();
drinkCoffee();
if (checkMail())
{
  answerMail();
}

and put that into morningRoutine(). Now we have

drinkTea();
morningBath();
if (checkMail())
{
  answerMail();
}

and call it sundayMorningRoutine(). But then there is duplicated code. Or expand morningRoutine(day) as

if (day == sunday){
  drinkTea();
  morningBath();
} else {
  morningBath();
  drinkCoffee();
}    
if (checkMail())
{
  answerMail();
}

or maybe

if (day == sunday){
  drink(Tea);
  morningBath();
} else {
  morningBath();
  drink(Coffee);
}    
if (checkMail())
{
  answerMail();
}

I wonder if that is good style.. maybe.. Thanks for that hint!

Another thing to try is to create a strategy pattern: pass in an array of function pointers and iterate over them. — duffymo, Jul 27 '12 at 11:31
@duffymo: What you describe is not strategy pattern, it is iterating over a bunch of function pointers. — Mare Infinitus, Jul 27 '12 at 11:51
@Mare: We have some functions extracted resulting in monsters like: "CheckElementConnectedAndNotFinishedAndNotNIO" versus "CheckElementConnectedAndFinished" versus "CheckElementConnectedAndNotNIO" and so on — Oliver, Jul 27 '12 at 12:16
@duffymo: That would require bigger changes. Function pointer are considered the root of all evil over here. — Oliver, Jul 27 '12 at 12:17
Actually, function pointers often lead towards the most elegant code. — Philip, Jul 27 '12 at 12:31

score 1 · Accepted Answer · answered Jul 27 '12 at 12:28

1

Regarding C code, it's perfectly normal to frequently encounter NULL pointer checks, especially when it comes to function arguments. Personally, I prefer to let the caller resolve the situation, as in:

if (p == NULL) {
    /* maybe do some cleanup and then: */
    return errcode;
}

Public functions, i.e. functions that are part of the API, should always check for NULL pointers. Functions that are designated static may IMO drop those checks. And finally, there's always assert(). Those checks can be suppressed by the compiler flag -NDEBUG. I use assert() in static functions instead of if-statements and in "public" functions for tests that reveal that the caller didn't actually understand the API as a whole, e.g. in a linked list lib:

void list_print(list **l)
{
    assert(l != NULL);    /* no valid list passed by reference can ever be NULL */

    if (*l == NULL)       /* but it can be empty */
        return;

    /* print list */
}

As for your second concern, I can see three options:

1) leave everything as it is - after all, it's working.

2) introduce new functions:

int function_1_2a_3();
int function_1_2b_3();

3) introduce new parametrized functions:

int function_1_2_3(int type);

Personally, I prefer the latter approach, but that is really just a matter of style.

answered Jul 27 '12 at 12:28

Philip

5,795
3
33
68

Well, as for the NULL-Pointer, I guess I have to get used to that. It's really anoying to see half the code is about NULL-Pointer checking. As for your three option: 1) But it's a pain in the ass to maintance 2) See my Comment above. We have some of this functions. There again DRY is violated, because they are composed by 80% the same code. To sort all that out will create one layer above the next. 3) That would be a possibility, but I fear to create the big one mammut that will be controlled be a myriade of switches, cases and stuff. – Oliver Jul 27 '12 at 12:55
...and it gets *really* annoying if you have an API function checking for `NULL` pointers which passes the arguments to some internal function which checks again, and so on. An expert programmer may be able to drop most of those checks, but for us mortals, it's probably wise to stick with redundancy. Three options: 1) Yep, maintenance easily comes in the way. 2) I don't see a violation of DRY: hide your original functions behind the `static` keyword and only expose your convenience layer, calling those other functions. I would call this approach "elegant". (no space left, see next comment) – Philip Jul 27 '12 at 13:19
3) It depends on the number of functions that you're trying to hide. If you already feel fear, you are probably right about that, and it's furthermore a bad idea with regard to maintenance. `ioctl()` is a good example of this technique, and a perfect example of a crappy interface. – Philip Jul 27 '12 at 13:21

score 1 · Answer 2 · answered Jul 27 '12 at 12:39

I agree a lot with what Philip said, but I want to add that one of the main points of Clean Code is to make the code read like English. If you encounter common seqeuences of functions and you can't give that sequence a good name, then it is better to leave it. For example, if you have

vacuumTheCarpet();
dustTheFurniture();
putThingsInTherePlace();

you might replace this with

cleanTheHouse();

but if you have

getTheMail();
eatSomeIceCream();
writeALetter();

you're probably better off leaving them as separate functions.

score 1 · Answer 3 · answered Jul 27 '12 at 13:39

For error checks, macros can make your code much cleaner:

#define CheckNullLogClean(ptr) if(ptr == NULL) { \
    status = ERR_NULL_PTR; \
    LogError(status); \
    goto cleanup; }

int func(int *input) {
    status = 0;
    CheckNullLogClean(input);
    Do_Things();
    cleanup:
    Release_Resources();
    return status;
}

A codebase I used to work on did something similar to this. And every function was set up so that it would return an integer called status (with a value in a module-scoped error code table) after a label called cleanup. If each function call had its return value checked, our log files would contain stack traces with file names and line numbers (LogError was a macro that called a function with the __FILE__ and __LINE__ macros). And we could use the same error checking macros across the whole project.

Oh, and if your functions serve similar purposes, perhaps a function pointer array that you iterate through would make sense.

How does DRY combine with "Separation of concern" and "One function, one task"?

3 Answers3