Re: [PATCH 02/15] read-cache: Improve readability

Stefan Beller <sbeller@google.com> writes:

Of course, things are not so black-and-white in the real world.  

You can also take an extreme stance and view

	if (!pretend && do_it_and_report_error())
        	error(...);

differently. I would explain that what the whole thing is trying to achieve as "'do it' part is the primary thing we want to do, but it only can be done when we are not pretending, and we show an error when the 'do it' part fails." and would suggest structuring it more like this:

	if (pretend)
        	; /* nothing */
	else if (do_it_and_report_error())
        	error(...);

or

	if (!pretend) {
        	if (do_it_and_report_error())
                	error(...);
	}

But you could say "The final objective of the whole thing is to show an error message, but if we are pretending or if our attempt to 'do it' succeeds, we do not have to show the error", and such a view may make sense depending on what that 'do it' is. The original may be justified under such an interpretation.

We may be tempted to write (note: each boolean term may be a call with many complex arguments)

    if (A && B && C && D && E)
	...

when it is in fact logically is more like this:

    /* does not make sense to attempt C when A && B does not hold */
    if (A && B) {
    	if (C && D && E)
        	...
    }

But it becomes a judgement call if splitting that into nested two if statements is better for overall readability when the top-level if statement has an else clause. You cannot turn it into

    /* does not make sense to attempt C when A && B does not hold */
    if (A && B) {
    	if (C && D && E)
        	...
    } else {
        ... /* this cannot be what the original's else clause did */
    }

blindly. It would need further restructuring. I think the code inside the refs.c is a result of making such judgement calls.

I dropped 2/15 and expect the real fix in the future; no rush, though.