Re: [PATCH] Simplify the code and avoid an attribution.

Mike Hommey [off-list ref] writes:

On Wed, Nov 21, 2007 at 11:00:02PM -0200, André Goddard Rosa wrote:

quoted

--- a/config.c
+++ b/config.c

@@ -447,15 +447,16 @@ int git_config_from_file(config_fn_t fn, const

char *filename)
 	int ret;
 	FILE *f = fopen(filename, "r");

-	ret = -1;
-	if (f) {
-		config_file = f;
-		config_file_name = filename;
-		config_linenr = 1;
-		ret = git_parse_file(fn);
-		fclose(f);
-		config_file_name = NULL;
-	}
+	if (!f)
+		return -1;
+
+	config_file = f;
+	config_file_name = filename;
+	config_linenr = 1;
+	ret = git_parse_file(fn);
+	fclose(f);
+	config_file_name = NULL;
+
 	return ret;
 }

Actually, since it is more likely that the file has been opened, the
original code is more optimal because it doesn't generate a jump in most
cases. And if you're worried about the ret variable, don't worry, it's
most likely stripped out by the compiler optimizations.

I did not comment on this patch partly because I did not know
what "attribution" meant.  I think it is a good change from
readability, not micro-optimization, point of view.

If you structure your code this way,

	do this
        if (there is an error)
        	return error code;
	do that
	do the rest

it is much easier to read than

	do this
        set error code pessimistically
        if (it was successful) {
        	do that
                update error code
                do the rest
	}
        return error code

especially the more likely part that runs the real processing
("do that...do the rest") tends to grow over time, and even
acquire other error checks.