Code quality

[Daniel T.]

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!

So I went ahead and ran your code though some tests. I was a little
surprised at the results, I thought of "subtraction" of rectangles more
like a set-intersection than a union... However I was especially
surprised at this particular result:


void failUnlessEqual( int a, int b ) {
assert( a == b );
}

void test_top_bottom_intersecting() {
region r1 = { 1, 3, 5, 7 };
region r2 = { 1, 3, 6, 8 };
int result = try_subtract_region( &r1, &r2 );
failUnlessEqual( 1, result );

failUnlessEqual( 1, r1.left );
failUnlessEqual( 3, r1.right );
failUnlessEqual( 5, r1.top );
failUnlessEqual( 6, r1.bottom );

failUnlessEqual( 1, r2.left );
failUnlessEqual( 3, r2.right );
failUnlessEqual( 6, r2.top );
failUnlessEqual( 8, r2.bottom );
}

Why is r1.bottom == 6 in this test?

 

[August Karlstrom]

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;
}

As others have mentioned, programming style is highly subjective.
However, here is an outline of how I would write it.

#include <stdbool.h>

typedef struct region Region;

/* Returns non-zero if the regions are disjoint. */

bool disjoint(Region *r1, Region *r2)
{
...
}

/* Returns non-zero if the first region covers the second. */

bool superset(Region *r1, Region *r2)
{
...
}

/*
* Returns non-zero if the difference between the regions is
* well-defined (the result is a rectangle).
*/
bool can_subtract(Region *r1, Region *r2)
{
...
}

/*
* Calculates the difference between the regions.
*
* Precondition: can_subtract(r1, r2).
*/
void get_difference(Region *r1, Region *r2, Region **result)
{
...
}


August

 

[Ben Pfaff]

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. [...]

Well, yes, but I usually find myself doing this on enormous trees
of code that I didn't write. For example, I believe that the
source tree that a company I contract to has 1 million lines or
more of code (all their software is in a single source control
repository). I don't necessarily know a return type or anything
else useful.

Anyway, it's not so useful as all that, I'll admit, especially
when (relatively smart) "tags" programs are available.

 

[void * clvrmnky()]

[...]
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.

However, if you are working at a shop where this is sort of thing is
done, it is best to follow along. I help maintain some code that is
nigh on 20 years old now, back when the more ubiquitous code management
tools were vi, find and grep. Searching for a function with find .
-name '*.c' | xargs grep "^functionName" was just a matter of course.

While our toolset has grown (and our C libraries shrink as Java eats up
their functionality) I still reach for find and grep when poking around
in code I have not visited in a while. Knowing your function
definitions are always in column 1 is pretty reliable, especially if you
have a code base that already conforms to this layout.

It's funny because now when I hack up a quick test my fingers
automatically type

int
fooTest(void) {
/* ... */
}

without even thinking about it.

That being said, I'm not religiously tied to this or not. Whitespace
normalization is more important to me. We have a shop where bugfix
changes are rejected if whitespace is messed up, and whitespace
corrections have to checked in on their own defect. I expect this is
pretty common at most shops.