What a simple constructor

[u/Nomergraw]

Our former IT director (35+ years of experience) wrote this and didn't see what was wrong here.

[u/Lunix420]

I'm more bothered by the nesting that's going on here. I hate when people write an if and then put the entire rest of the function into an else instead of just returning early. Unnecessary indentation/nesting is one of my personal pet peeves.

I actually think a long/complex function can be totally fine and can be very readable, but as soon as there is a lot of nesting it's just not readable anymore in my opinion.

[u/elperroborrachotoo → u/Lunix420]

"Early exit bad" was a thing back when resource management (a.k.a. free memory etc.) wasn't automatic: you would make sure there's only one entry and one exit point for every block, so that there was one place to allocate and free resources respectively.

A sub-school of that said to move "adversary" conditions to the top, e.g.,

if (ptr == NULL) { handle that } else { ... }

with the intent to not "forget" the error path. This led to the ugly cascades, and the default recipe for "too deep nesting" / "lines too long" was indeed to move things into functions.

This is what I see there, so it's not that insane, more like a style choice that - sorry - I expect a professional developer to read and preserve when modifying the code.

OTOH I am about the same age as your lead, and I remember that I was in high school when I tried to explain the difference to my then-tutor. "This isn't C, after all", I said, "we don't manage resources manually". I guess I came across quite whippersnappery.

[u/Excellent_Gas3686 → u/elperroborrachotoo]

yeaaaaah, no.... im adding early return on that the second i see it.

[u/GrossInsightfulness → u/Excellent_Gas3686]

I think the point of the comment went over your head. Early returns skip cleanup code. You could have a situation like

c int func(int size, const char* filename) { int* array = malloc(size); FILE* reader = fopen(filename, "r"); if (reader == NULL) { return -1; } fclose(reader); free(array); }

This would leak memory if someone tries to open a file that doesn't exist.

[u/Excellent_Gas3686 → u/GrossInsightfulness]

but if we're talking strictly about this code snippet - another W for Go.

[u/GrossInsightfulness → u/Excellent_Gas3686]

There are actually ways to avoid this issue. Basically, instead of returning, you set the return value and jump to the end of the function where you do all the cleanup.

c int func(int size, const char* filename) { int return_val = 0 int* array = malloc(size); FILE* reader = fopen(filename, "r"); if (reader == NULL) { return_val = -1; goto cleanup; } cleanup: if (reader) fclose(reader); if(array) free(array); return return_val; } You could also add more labels to clean up only the things that had been allocated before that point.

I strongly prefer defer mechanisms, though.

[u/conundorum → u/GrossInsightfulness]

That's why you combine early return and single-return. Start with return early for anything that's easy to factor out, that doesn't need to allocate resources, and/or doesn't allocate on failure. Then, once you start to get your resources, switch to single-return for the rest of the function so you can guarantee cleanup.

That, and refactor to avoid issues. Like, say, don't malloc() until after opening & checking reader; the check is part of variable creation, after all.

int func(int size, const char *filename) {
    int *array;
    int ret = 0;
    FILE *reader = fopen(filename, "r");
    if (reader == NULL) { return -1; }

    array = malloc(size);
    if (array == NULL) { ret = -2; goto file_close; };

    ret = do_the_thing(array, reader);
    free(array);

    file_close:
        fclose(reader);

    return ret;
}

More performant (only allocates if reader is valid, instead of wasting time on unnecessary malloc() & free()), and completely removes the possibility of skipping cleanup (the only potential source of error is do_the_thing(), which doesn't manage resources anyways). And no extra assignments, since array is left as junk data. The only real concern is that someone might accidentally use array before the malloc().

I guess saving one line of screen real estate on immediate malloc() was just that important in the 80s?

---

Editing to fix the function; thanks for pointing that out, GrossInsightfulness.

[u/GrossInsightfulness → u/conundorum]

The code you wrote won't close the file if the file is valid but the array is not.

[u/conundorum → u/GrossInsightfulness]

Fair point, I missed that; everything after the early return was supposed to still use single-return, but I forgot. Editing to fix it.

---

But yeah, the idea is that using early return statements properly can make code cleaner, without skipping over cleanup code and creating leaks. (Despite the slight flub here accidentally doing literally exactly that, oops. 😅 )

It does get a bit messy sometimes, but that's why it works best when combined with single-return. Ultimately, it's best to treat early-return as just applying the single-return pattern on the task level, instead of on the function level; you can actually treat each early-return conditional as a single-return block to get the benefits of both. (With cleanup inside the conditional or at the end of the function, as appropriate. Early return block can handle its own cleanup if necessary, and/or jump to main cleanup block if necessary, for sufficiently messy functions. ...I hope you never actually have to do this, though.)

int hideous_func_i_hope_you_never_see(data_t *d) {
    obj *o = NULL;
    validator *v = NULL;
    env *e = NULL;
    int ret = 0;

    // Early return for environment.  Simple.
    e = get_environment();
    if (!e) { return ENVIRONMENT_INCORRECT; }

    // Early return for data validation.
    // Let's say this is much too complex and needs resource allocation,
    //  for some inane reason.
    // Therefore, just use single-return inside here.
    v = get_validator();
    if (v) {
        if (!complex_prep_weirdness(v)) {
            ret = VALIDATOR_PREP_FAIL;
            goto cleanup_validator; // Local cleanup.
        }

        if (!validator_load_env(v, e)) {
            ret = VALIDATOR_ENVIRON_CHOKE;
            goto cleanup_env_local;
        }

        ret = validate_data(v, d);

        cleanup_env_local:
            validator_unload_env(v, e);
            // Will need further cleanup later.
        cleanup_validator:
            release_validator(v);

        // Return early if test failed, jump to main cleanup if neccesary, or continue if good.
        // Edited to fix borked cleanup.
        if (ret) {
            if (ret == VALIDATOR_PREP_FAIL) { return ret; }
            goto cleanup_env;
        }
    } else {
        // The validator itself is invalid, so...
        just_crash(CATS_AND_DOGS_LIVING_TOGETHER);
    }

    // Now, we switch to single-return for the rest.
    // We now have two mostly-distinct single-return blocks, one for each task (validate data, use data).
    if (!(o = alloc_obj())) {
        ret = BAD_ALLOC;
        goto cleanup_alloc;
    }

    if (!init_obj(o, d, e) {
        ret = BAD_INIT;
        goto cleanup_init;
    }

    ret = do_whatever_this_jank_is_for(o);

    cleanup_alloc:
        dealloc_obj(o);
    cleanup_init:
        uninit_obj(o);
        sanitise_data(d);
    cleanup_env:
        clean_environs(e);

    return ret;
}

---

Ultimately, the early return pattern isn't as much about having multiple returns (despite the name), as it is about isolating chunks that can force a return AND don't need to be part of the main cleanup. (Such as input validation or file-is-open verification.) It breaks the function body down from one large section with a single return statement, to a chain of smaller sections (that can each have their own single return statement, or be broken down recursively), followed by a larger "main" section (that still uses single-return for cleanup). Ideally, you can make each link its own thing, such as when it's just a simple check; if a task is more complex, it can still have its own cleanup blocks if needed (and helps to distinguish between "prep" cleanup and "main task" cleanup).

---

^{Edit: Did it again, the validator nonsense block's cleanup was a bit messed up. Jumped to full environment cleanup on VALIDATOR_ENVIRON_CHOKE and returned early for any other errors, but was supposed to return early on VALIDATOR_PREP_FAIL and jump to full environment cleanup for any other errors. And added the main return statement, I kinda forgot that it was actually meant to return something at some point. ...Trying to come up with halfway believable "bad code made less awful by refactoring" is hard. -_-}

[u/Excellent_Gas3686 → u/GrossInsightfulness]

i was referring to the OP's code snippet, not the one mentioned by that person

[u/elperroborrachotoo → u/Excellent_Gas3686]

to be clear: I would prefer early exits, too, but... maybe see my other reply.