[Cluster-devel] [GFS2 PATCH] GFS2: eliminate gfs2_log_write_bh and simplify

[Bob Peterson]

Hi,

----- Original Message -----
| On 01/12/17 17:31, Bob Peterson wrote:
| > Hi,
| >
| > This patch makes no functional changes. Its goal is to simplify the
| > code and make function gfs2_before_commit more readable. It does
| > this by eliminating function gfs2_log_write_bh which was a one-line
| > function that simply called gfs2_log_write. It simplifies things
| > by introducing a variable bh to reduce the references to bd2->bd_bh.
| >
| > Signed-off-by: Bob Peterson <rpeterso at redhat.com>
| While using the new bh variable might improve things, I'm not at all
| convinced that eliminating gfs2_log_write_bh is a good plan, since it is
| a useful helper that is self-documenting,
| 
| Steve.

I guess it comes down to coding preference. The official coding-style
documentation doesn't give a suggested minimum size of a function;
it only cautions against them being too long. It says:

"They should fit on one or two screenfuls of text (the ISO/ANSI screen
size is 80x24, as we all know)"

But IMHO, one 24-line function is much preferred to 24 lines broken into
six single-line, single-call functions like:

Function a()
{
   b();
}

If a function gets too long, it is difficult to understand, but
IMHO the opposite is also true: If there are too many functions, the
reader's brain suffers from "death by a thousand cuts."

There's also the argument against "jumping around in the code."
If I read through a function of 24 lines, it makes some logical sense.
If I can't read it through because I need to keep interrupting the
flow by jumping to the sub-functions to analyze what they do, it makes
it twice as hard to understand.

In my way of thinking, a one-line function that simply calls another
function is especially useless when it is only called in one place like
this one. The only thing it achieves is to obfuscate the code and make
it harder for maintainers to understand. If it's called in two places,
then sure; it's useful.

IOW, a one-line self-documenting helper doesn't make the code more clear;
it only adds to the levels of indirection needed in one's brain to keep
them all straight. It only points out the shortcomings of the calling
function to make things clear.

Having all these tiny little functions may seem helpful as they
break the code down into more digestible morsels. But in the long run,
it's harder because it becomes impossible to keep all the helpers
straight.

Would it be more acceptable if I moved the explanation comment before
the call? So instead of:

/**
 * gfs2_log_write_bh - write a buffer's content to the log
 * @sdp: The super block
 * @bh: The buffer pointing to the in-place location
 * 
 * This writes the content of the buffer to the next available location
 * in the log. The buffer will be unlocked once the i/o to the log has
 * completed.
 */

static void gfs2_log_write_bh(struct gfs2_sbd *sdp, struct buffer_head *bh)
{
	gfs2_log_write(sdp, bh->b_page, bh->b_size, bh_offset(bh));
}
(-----------------long span of code to interrupt your brain---------------)
 			} else {
				gfs2_log_write_bh(sdp, bd2->bd_bh);

to something more succinct like this?

 			} else {
				/* write the content of the buffer to the
				   next available location in the log. The
				   buffer will be unlocked once the i/o to
				   the log has completed.
				 */
				gfs2_log_write(sdp, bd2->bd_bh);


Regards,

Bob Peterson