Code quality

[Edward Gregor]

Hi!
I've written this code as a part of my program and I
wonder if you would mind looking at the
try_subtract_region and tell me if it well written.
Thankful for help!

struct region {
int left, right;
int top, bottom;
};

/* Return 1 if the two regions are intersecting */
static int intersect(struct region *r1, struct region *r2)
{
return (r2->right > r1->left && r2->bottom > r1->top &&
r1->right > r2->left && r1->bottom > r2->top);
}

/* Return 1 if r1 is covering the whole r2 regin */
static int covering(struct region *r1, struct region *r2)
{
return (r1->left <= r2->left && r1->right >= r2->right &&
r1->top <= r2->top && r1->bottom >= r2->bottom);
}

/* Try to subtract r2 from r1. Only subtract if the resulting region
* is a rectangle, otherwise, do nothing.
* Returns 1 on successful subraction, and 0 if nothing is done. */
static int try_subtract_region(struct region *r1, struct region *r2)
{
/* If the regions are not intersecting each other, then
* we have nothing to do. */
if (!intersect(r1, r2)) return 0;
/* Same goes if r2 is covering the whole area. */
if (covering(r2, r1)) return 0;

/* Since region 2 is not covering the whole region 1, we can make
* certain assumtions, that is, if region 2 is more to the right, left
* and bottom than region 1, it won't cover the top and we
* remove the bottom part of region 1. */

/* r2 is wider */
if (r2->left <= r1->left && r2->right >= r1->right) {
if (r2->top <= r1->top) { /* r2 is covering the top part */
r1->top = r2->bottom;
return 1;
}
else if (r2->bottom >= r1->bottom) { /* r2 is covering the
bootom */
r1->bottom = r2->top;
return 1;
}
}
/* r2 is taller */
else if (r2->top <= r1->top && r2->bottom >= r1->bottom) {
if (r2->left <= r1->left) { /* r2 is covering the left part */
r1->left = r2->right;
return 1;
}
else if (r2->right >= r1->right) { /* r2 is covering the right
part */
r1->right = r2->right;
return 1;
}
}

/* If we got here, we couldn't do nothing */
return 0;
}

 

[Diomidis Spinellis]

I've written this code as a part of my program and I
wonder if you would mind looking at the
try_subtract_region and tell me if it well written.

Programming style is a highly subjective issue, and comments are bound
to generate flames, but here are some ideas.

> struct region {
> int left, right;
> int top, bottom;
> };

Add a comment saying what this structure represents. Are the limits
inclusive or exclusive?

>
> /* Return 1 if the two regions are intersecting */
> static int intersect(struct region *r1, struct region *r2)

Split into two lines (the same throughout the program):
static int
intersect(struct region *r1, struct region *r2)

> {
> return (r2->right > r1->left && r2->bottom > r1->top &&
> r1->right > r2->left && r1->bottom > r2->top);
> }
>
> /* Return 1 if r1 is covering the whole r2 regin */
> static int covering(struct region *r1, struct region *r2)
> {
> return (r1->left <= r2->left && r1->right >= r2->right &&
> r1->top <= r2->top && r1->bottom >= r2->bottom);
> }
>
> /* Try to subtract r2 from r1. Only subtract if the resulting region
> * is a rectangle, otherwise, do nothing.
> * Returns 1 on successful subraction, and 0 if nothing is done. */

Format block comments as:
/*
* Try to subtract r2 from r1. Only subtract if the resulting region
* is a rectangle, otherwise, do nothing.
* Returns 1 on successful subraction, and 0 if nothing is done.
*/

> static int try_subtract_region(struct region *r1, struct region *r2)
> {
> /* If the regions are not intersecting each other, then
> * we have nothing to do. */
> if (!intersect(r1, r2)) return 0;
> /* Same goes if r2 is covering the whole area. */
> if (covering(r2, r1)) return 0;

My opinion is that the two comments above are excessive, but this is
subjective, and depends on who will read your code. OS kernel hackers
would regard them as noice; in a homework project they show you
understand what you are doing.

> /* Since region 2 is not covering the whole region 1, we can make
> * certain assumtions, that is, if region 2 is more to the right, left
> * and bottom than region 1, it won't cover the top and we
> * remove the bottom part of region 1. */
>
> /* r2 is wider */
> if (r2->left <= r1->left && r2->right >= r1->right) {
> if (r2->top <= r1->top) { /* r2 is covering the top part */
> r1->top = r2->bottom;
> return 1;
> }

The two comments above are inconsistently placed: one before the if, one
after the if.

Also, in the style you are using, you should be formatting cascading if
.... else if sequences as:
} else if (...) {

 

[Edward Gregor]

Programming style is a highly subjective issue, and comments are bound
to generate flames, but here are some ideas.

Add a comment saying what this structure represents. Are the limits
inclusive or exclusive?

Sorry but what do you mean by inclusive and exclusive limits?

Thank you for your help!

 

[Keith Thompson]

I've written this code as a part of my program and I
wonder if you would mind looking at the
try_subtract_region and tell me if it well written.

Programming style is a highly subjective issue, and comments are bound
to generate flames, but here are some ideas.
[snip]

/* Return 1 if the two regions are intersecting */
static int intersect(struct region *r1, struct region *r2)

Split into two lines (the same throughout the program):
static int
intersect(struct region *r1, struct region *r2)

As you say, this is a highly subjective matter of style; on this
particular point my sense of style differs from yours.

I prefer to put the function type and name on one line. I see a lot
of code that puts the function name at the beginning of a line; I
*think* it's because some tools happen to work better with that
layout. You could argue that the name of the function is the most
important piece of information, and that putting it at the beginning
of a line makes it stand out. That's a valid point (hmm, what's the
oppposite of a "strawman argument"?), but I find it artificial and
clumsy.

And I note that neither K&R nor the standard does this.

Of course if your organization requires this style, or if you're
working on code that already uses it, you should follow it.

[snip]

Also, in the style you are using, you should be formatting cascading
if ... else if sequences as:
} else if (...) {

Again, my own style differs. In the style I prefer, an opening brace
goes at the end of a line (except for the outer brace of a function
definition), and a closing brace goes at the beginning of a line *by
itself*. For example:

if (condition1) {
/* ... */
}
else if (condition2) {
/* ... */
}
else {
/* ... */
}

I'm not sure there's a really good reason not to put the "else" or
"else if" on the same line as the "}", other than that it's what I'm
used to. Both styles are common, and both are internally consistent.

Another style point: I always use braces on conditional statements,
even when there's only one statement being controlled. For example, I
write this:

if (condition) {
statement;
}

never this:

if (condition)
statement;

The only time I might break this rule is if I put the whole thing on
one line, which I rarely do:

if (condition) statement;

(This is a habit I picked up from Perl, which requires the braces, but
I find it a good idea for C as well.)

 

[clayne]

Programming style is a highly subjective issue, and comments are bound
to generate flames, but here are some ideas.

I think the original poster was actually asking about quality of the
code in so far as what it actually does - and not the formatting of
comments. Splitting function declarations and initial comment structure
do not make high quality code on their own. It's very easy to write
nicely formatted crap.

 

[Ben C]

[...]
Split into two lines (the same throughout the program):
static int
intersect(struct region *r1, struct region *r2)

As you say, this is a highly subjective matter of style; on this
particular point my sense of style differs from yours.

I prefer to put the function type and name on one line. I see a lot
of code that puts the function name at the beginning of a line; I
*think* it's because some tools happen to work better with that
layout.

I think it is (or was) recommended by the GNU coding standards, and I
think the reason given was that some tools or other look for the symbol
name in column 1.

I don't like it either. And whatever these tools are, isn't the proper
solution to fix the tool? It doesn't seem that should be too hard, and
would also result in a more reliable tool.

I also wonder if the GNU coding standards aren't deliberately a bit
unorthodox to emphasize the fact that it is all new code and none of it
is pasted from anywhere that is copyrighted by someone.

The Open Solaris sources, for example, are laid out in a way that's much
closer to the "K&R" style.

[snip]

Another style point: I always use braces on conditional statements,
even when there's only one statement being controlled. For example, I
write this:

if (condition) {
statement;
}

I highly subjectively really don't like that

never this:

if (condition)
statement;

The only time I might break this rule is if I put the whole thing on
one line, which I rarely do:

if (condition) statement;

Quite a few people I've met (excluding myself) don't like single-line
conditionals, on the grounds that they can't set a breakpoint on the
conditionally-executed statement.

(This is a habit I picked up from Perl, which requires the braces, but
I find it a good idea for C as well.)

I kept getting it wrong in Perl.

 

[Keith Thompson]

I highly subjectively really don't like that

Fair enough. I'll just mention one advantage of always using braces:
it makes it easier to add another statement. The above is easily
changed to:

if (condition) {
printf("Here we are\n");
statement;
}

whereas without the braces, it's too easy to change:

if (condition)
statement;

to

if (condition)
printf("Here we are\n");
statement;

If you're sure you'll never make that mistake, either because you're
supremely self-disciplined or because you use an editor that
automatically formats your code for you, that's great -- assuming you
can be certain that nobody who maintains your code will make that
mistake either.

Then again, the convention of writing "if (0 == x)" rather than
"if (x == 0)" is also designed to minimize errors, and I don't use it
because I (subjectively) find it unutterably ugly -- worse than
Hungarian notation.

De gustibus non est disputandum (never argue with a bus driver named
Gus).

 

[Ian Collins]

[...]

Another style point: I always use braces on conditional statements,
even when there's only one statement being controlled. For example, I
write this:

if (condition) {
statement;
}

I highly subjectively really don't like that

Fair enough. I'll just mention one advantage of always using braces:
it makes it easier to add another statement. The above is easily
changed to:

if (condition) {
printf("Here we are\n");
statement;
}

whereas without the braces, it's too easy to change:

if (condition)
statement;

to

if (condition)
printf("Here we are\n");
statement;

If you're sure you'll never make that mistake, either because you're
supremely self-disciplined or because you use an editor that
automatically formats your code for you, that's great -- assuming you
can be certain that nobody who maintains your code will make that
mistake either.

Then again, the convention of writing "if (0 == x)" rather than
"if (x == 0)" is also designed to minimize errors, and I don't use it
because I (subjectively) find it unutterably ugly -- worse than
Hungarian notation.

Good use of unit tests, preferably written before the code, is a good
way to prevent both of the above ailments.

 

[pete]

I highly subjectively really don't like that


Quite a few people I've met (excluding myself) don't like single-line
conditionals, on the grounds that they can't set a breakpoint on the
conditionally-executed statement.


I kept getting it wrong in Perl.

Using a compound statement,
simplifies adding more statements to the if statement,
which is something that happens often enough.
It breaks up optical illusions
which can be caused by indentation.

if (condition)
statement;
statement;

 

[Andrew Poelstra]

Fair enough. I'll just mention one advantage of always using braces:
it makes it easier to add another statement. The above is easily
changed to:

if (condition) {
printf("Here we are\n");
statement;
}

whereas without the braces, it's too easy to change:

if (condition)
statement;

to

if (condition)
printf("Here we are\n");
statement;

If you're sure you'll never make that mistake, either because you're
supremely self-disciplined or because you use an editor that
automatically formats your code for you, that's great -- assuming you
can be certain that nobody who maintains your code will make that
mistake either.

I never considered that the people revising my code could make that
mistake; I personally never make it because my tabbing style is very strict.

I find that excessive symbols makes text difficult to read (but I am
scarred by the Mavis Beacon lessons taught to me in school, so perhaps
I'm just biased).

Then again, the convention of writing "if (0 == x)" rather than
"if (x == 0)" is also designed to minimize errors, and I don't use it
because I (subjectively) find it unutterably ugly -- worse than
Hungarian notation.

Agreed.

 

[Ben Pfaff]

I think it is (or was) recommended by the GNU coding standards, and I
think the reason given was that some tools or other look for the symbol
name in column 1.

It makes it easy to find function definitions with "grep", which
is occasionally useful.

 

[Phlip]

struct region {
int left, right;
int top, bottom;
};

Where's it's unit tests?

 

[Richard Heathfield]

Edward Gregor said:

Sorry but what do you mean by inclusive and exclusive limits?

I would be very much surprised if these were not supposed to be inclusive.

If left is 1, right is 9, top is 3 and bottom is 6, then "inclusive" means
that the region is nine units wide and four high - i.e.

111
0123456789012
0.............
1.............
2.............
3.xxxxxxxxx...
4.xxxxxxxxx...
5.xxxxxxxxx...
6.xxxxxxxxx...
7.............
8.............

but "exclusive" means that the left and right values themselves are not part
of the region (and similarly that the top and bottom are not). This means
the region is two rows shorter and two columns narrower:

111
0123456789012
0.............
1.............
2.............
3.............
4..xxxxxxx....
5..xxxxxxx....
6.............
7.............
8.............

 

[Logan Shaw]

Edward Gregor said:


I would be very much surprised if these were not supposed to be inclusive.

If left is 1, right is 9, top is 3 and bottom is 6, then "inclusive" means
that the region is nine units wide and four high - i.e.

111
0123456789012
0.............
1.............
2.............
3.xxxxxxxxx...
4.xxxxxxxxx...
5.xxxxxxxxx...
6.xxxxxxxxx...
7.............
8.............

but "exclusive" means that the left and right values themselves are not part
of the region (and similarly that the top and bottom are not). This means
the region is two rows shorter and two columns narrower:

It could be quite useful to make one set of limits inclusive and the other
exclusive. Left and top could be inclusive and right and bottom exclusive.
This has the nice property that if region1 and region2 are exactly adjacent
to each other horizontally (and region1 is the one to the left), then
region1.right == region2.left. If everything is defined as inclusive,
then testing whether things abut each other exactly gets uglier.

- Logan

 

[Richard Heathfield]

Ben Pfaff said:

It makes it easy to find function definitions with "grep", which
is occasionally useful.

Perhaps it's just me, but I have no difficulty picking out a function
definition from a pageful of possibles. And if there's more than a pageful,
one can always pipe the grep through another grep. For example, in one of
my libraries,

grep -n Log *c

gives 518 matches, but

grep -n Log *c | grep -w int

gives just four.

 

[Richard Heathfield]

Logan Shaw said:

It could be quite useful to make one set of limits inclusive and the other
exclusive. Left and top could be inclusive and right and bottom
exclusive. This has the nice property that if region1 and region2 are
exactly adjacent to each other horizontally (and region1 is the one to the
left), then
region1.right == region2.left. If everything is defined as inclusive,
then testing whether things abut each other exactly gets uglier.

A slightly more intuitive way to capture that feature is to record width and
height rather than right and bottom, so that you'd have something like:

if(s1->left + s1->width == s2->left)
{
if(s1->top + s1->height >= s2->top &&
s2->top + s2->height >= s1->top)
{
/* s2 abuts, and is right of, s2, modulo bugs */

 

[James Dow Allen]

Edward Gregor said:

If left is 1, right is 9, top is 3 and bottom is 6, then "inclusive" means
that the region is nine units wide and four high - i.e.
...
but "exclusive" means that ...
the region is two rows shorter and two columns narrower...

In other words, the width, nominally right(9) subtract left(1)
is either too big (9) or too small (7) :-( :-(

Much better is to imitate MacIntosh QuickDraw (!) where

coordinates refer to the infinitely thin lines between pixel locations.
An actual pixel is drawn in the space to the immediate right and below
the coordinate. This eliminates the so-called "endpoint paranoia"
and associated off-by-one errors

James Dow Allen

 

[Rod Pemberton]

Fair enough. I'll just mention one advantage of always using braces:
it makes it easier to add another statement. The above is easily
changed to:

if (condition) {
printf("Here we are\n");
statement;
}

Having used that method and seeing the resultant problems, I highly
subjectively really don't like that, with the braces at the end of the
condition, it's too easy to change:

if (condition) {
printf("Here we are\n");
statement;
}

to

if (condition) {
printf("Here we are\n");
statement;
}

Which can completely obscure the block, if multiple if's are involved. I
would suggest this very beginner like method instead:

if (condition)
{
printf("Here we are\n");
statement;
}

It's easy to find the matching parens and is less likely to be indented
improperly.


Rod Pemberton

 

[Bill Pursell]

It makes it easy to find function definitions with "grep", which
is occasionally useful.

I suspect it also makes ^] commands faster (move to tag), since
the search can be anchored against the \n.

 

[Ben C]

Fair enough. I'll just mention one advantage of always using braces:
it makes it easier to add another statement. The above is easily
changed to:

if (condition) {
printf("Here we are\n");
statement;
}

whereas without the braces, it's too easy to change:

if (condition)
statement;

to

if (condition)
printf("Here we are\n");
statement;

Yup, I realize that's the point of it. The time you really must use
braces for single-line conditions is the "dangling else" situation:

if (x == 1)
1;
else if (x > 2)
if (x == 3)
2;
else
3;

vs.

if (x == 1)
1;
else if (x > 2)
if (x == 3)
2;
else
3;

which are both exactly the same to the poor compiler, and both
ambiguous.

Then again, the convention of writing "if (0 == x)" rather than
"if (x == 0)" is also designed to minimize errors, and I don't use it
because I (subjectively) find it unutterably ugly -- worse than
Hungarian notation.

I really can't stand that (0 == x) practice either!