6
\$\begingroup\$

I'm debating which version would be more suitable in parameters of code length, readability and maintenance. This is a portion of a set of functions which together parse a Gif file.

Version 1:

if ((errno = GifLoadHeaders(g, &bufTemp)) != GIF_SUCCESS) {
    GifDispose(g);
    *gPtr = NULL;

    return errno;
}

if ((errno = GifLoadGCT(g, &bufTemp)) != GIF_SUCCESS) {
    GifDispose(g);
    *gPtr = NULL;

    return errno;
}

if ((errno = GifLoadImages(g, &bufTemp)) != GIF_SUCCESS) {
    GifDispose(g);
    *gPtr = NULL;

    return errno;
}

Version 2:

if ((errno = GifLoadHeaders(g, &bufTemp)) != GIF_SUCCESS
    || (errno = GifLoadGCT(g, &bufTemp)) != GIF_SUCCESS
    || (errno = GifLoadImages(g, &bufTemp)) != GIF_SUCCESS) {

    GifDispose(g);
    *gPtr = NULL;

    return errno;
}

I generally prefer the latter version. Maybe there's a more optimal third solution to my issue. Any help?

EDIT: I'm sorry I did not include the entire routine. The code I've included is used in the function GifParse which initializes a given pointer to a Gif structure, which is named gPtr.

\$\endgroup\$
4
\$\begingroup\$

I like your second option better than the first, and I like @DaveJarvis answer best. There are times when neither of these options is possible or elegant. For example when the called functions have many parameters your 2nd option gets ugly. And when there is more than one resource, the separate function design can be tricky.

Just for the sake of it, here is another option:

int status = GifLoadHeaders(g, &buf);
if (status == GIF_SUCCESS) {
    status = GifLoadGCT(g, &buf);
}
if (status == GIF_SUCCESS) {
    status = GifLoadImages(g, &buf);
}
if (status != GIF_SUCCESS) {
    GifDispose(g);
    *gPtr = NULL;
    return status;
}
// etc

You can of course start nesting things, but that usually gets unreadable after a few levels. My example above might be easier to step through in a debugger than your 2nd option.

Note the use of status instead of the global errno. The errno variable is a bit special and should not normally not be written by you (and certainly not to anything other than 0). Also I replaced bufTemp with the shorter buf for readability.

\$\endgroup\$
4
\$\begingroup\$

Without knowing the context, I'd write it as:

if( (errno = GifLoad( g, &bufTemp )) == GIF_SUCCESS ) {
  // Process the correctly loaded image as necessary.
  GifProcess( g );
}

// Release memory once processing is complete.
GifDispose( g );
*gPtr = NULL;

// Single exit point to the function.
return errno;

Then write a wrapper for GifLoad that calls the three other functions.

This might not be a suitable solution if, for example, the processing is part of an interactive program wherein the processing is indeterminate. If processing cannot be coded at this point in the source, then consider:

if( (errno = GifLoad( g, &bufTemp )) != GIF_SUCCESS ) {
  // Perform whatever is necessary to inform the user the GIF cannot be loaded.
  GifInvalid( g );

  // Release memory.
  GifDispose( g );
  *gPtr = NULL;
}

return errno;

In both cases consider abstracting the three function calls into a single function, which cleans up the code and expresses the intent of those three function calls more concisely.

\$\endgroup\$
  • \$\begingroup\$ I'm sorry I did not include the entire routine. The codes I included are used in the function GifParse which initializes a given pointer to a Gif structure, which is named gPtr. \$\endgroup\$ – Novak Nov 25 '13 at 23:15
2
\$\begingroup\$

I would probably go with the second version if I had to choose among the two, with a slight variation like this:

bool success = (/* your if conditions here */)
if (success) {
    // do your stuff here
}

You could also use a function to check the condition, that way it is easier to add other conditions later. I had a similar thought when suggesting the bool.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.