Code quality

[Richard Heathfield]

Re: Code quality

Logan Shaw said:
[color=blue]
> 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.[/color]

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 */

--
Richard Heathfield
"Usenet is a strange place" - dmr 29/7/1999

http://www.cpax.org.uk

email: rjh at above domain (but drop the www, obviously)

[James Dow Allen]

Re: Code quality


Richard Heathfield wrote:[color=blue]
> Edward Gregor said:[color=green]
> > Sorry but what do you mean by inclusive and exclusive limits?[/color]
>
> 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...[/color]

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[color=blue]
> 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[/color]

James Dow Allen

[Rod Pemberton]

Re: Code quality


"Keith Thompson" <kst-u@mib.org> wrote in message
news:lnbqut5cuf .fsf@nuthaus.mi b.org...[color=blue]
> Ben C <spamspam@spam. eggs> writes:[color=green]
> > On 2006-04-22, Keith Thompson <kst-u@mib.org> wrote:[/color]
> [...][color=green][color=darkred]
> >> 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;
> >> }[/color]
> >
> > I highly subjectively really don't like that :)[/color]
>
> 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;
> }
>[/color]

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]

Re: Code quality


Ben Pfaff wrote:[color=blue]
> Ben C <spamspam@spam. eggs> writes:
>[color=green]
> > On 2006-04-22, Keith Thompson <kst-u@mib.org> wrote:[color=darkred]
> >> 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.[/color]
> >
> > 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.[/color]
>
> It makes it easy to find function definitions with "grep", which
> is occasionally useful.[/color]

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

[Ben C]

Re: Code quality

On 2006-04-23, Keith Thompson <kst-u@mib.org> wrote:[color=blue]
> Ben C <spamspam@spam. eggs> writes:[color=green]
>> On 2006-04-22, Keith Thompson <kst-u@mib.org> wrote:[/color]
> [...][color=green][color=darkred]
>>> 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;
>>> }[/color]
>>
>> I highly subjectively really don't like that :)[/color]
>
> 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;[/color]

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.
[color=blue]
> 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.[/color]

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

[Daniel T.]

Re: Code quality

In article <Osw2g.53998$d5 .208573@newsb.t elia.net>,
Edward Gregor <edwgre@hotmail .com> wrote:
[color=blue]
> Hi!
> I've written this code as a part of my program and I
> wonder if you would mind looking at the
> try_subtract_re gion and tell me if it well written.
> Thankful for help![/color]


So I went ahead and ran your code though some tests. I was a little
surprised at the results, I thought of "subtractio n" 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_re gion( &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?

[color=blue]
>
> struct region {
> int left, right;
> int top, bottom;
> };
>
> /* Return 1 if the two regions are intersecting */
> static int intersect(struc t 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_re gion(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;
> }[/color]

[August Karlstrom]

Re: Code quality

Edward Gregor wrote:[color=blue]
> Hi!
> I've written this code as a part of my program and I
> wonder if you would mind looking at the
> try_subtract_re gion 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(struc t 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_re gion(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;
> }[/color]

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(Re gion *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]

Re: Code quality

Richard Heathfield <invalid@invali d.invalid> writes:
[color=blue]
> Ben Pfaff said:
>[color=green]
>> Ben C <spamspam@spam. eggs> writes:
>>[color=darkred]
>>> On 2006-04-22, Keith Thompson <kst-u@mib.org> wrote:
>>>> 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.[/color]
>>
>> It makes it easy to find function definitions with "grep", which
>> is occasionally useful.[/color]
>
> 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. [...][/color]

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.
--
Here's a tip: null pointers don't have to be *dull* pointers!

[void * clvrmnky()]

Re: Code quality

Ben C wrote:[color=blue]
> On 2006-04-22, Keith Thompson <kst-u@mib.org> wrote:[color=green]
>> Diomidis Spinellis <dds@aueb.gr> writes:[color=darkred]
>>> [...]
>>> Split into two lines (the same throughout the program):
>>> static int
>>> intersect(struc t region *r1, struct region *r2)[/color]
>> 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.[/color]
>
> 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.
>[/color]
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 "^functionN ame" 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.