By using this site, you agree to our updated Privacy Policy and our Terms of Use. Manage your Cookies Settings.
429,615 Members | 1,961 Online
Bytes IT Community
+ Ask a Question
Need help? Post your question and get tips & solutions from a community of 429,615 IT Pros & Developers. It's quick & easy.

Code quality

P: n/a
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;
}
Apr 22 '06 #1
Share this Question
Share on Google+
23 Replies


P: n/a
Edward Gregor wrote:
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 (...) {

--
Diomidis Spinellis
Code Quality: The Open Source Perspective (Addison-Wesley 2006)
http://www.spinellis.gr/codequality?clc
Apr 22 '06 #2

P: n/a
Diomidis Spinellis wrote:
Edward Gregor wrote:
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?


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

Thank you for your help!
Apr 22 '06 #3

P: n/a
Diomidis Spinellis <dd*@aueb.gr> writes:
Edward Gregor wrote:
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.)

--
Keith Thompson (The_Other_Keith) ks***@mib.org <http://www.ghoti.net/~kst>
San Diego Supercomputer Center <*> <http://users.sdsc.edu/~kst>
We must do something. This is something. Therefore, we must do this.
Apr 22 '06 #4

P: n/a
Diomidis Spinellis wrote:
Edward Gregor wrote:
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.


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. :)

Apr 23 '06 #5

P: n/a
On 2006-04-22, Keith Thompson <ks***@mib.org> wrote:
Diomidis Spinellis <dd*@aueb.gr> writes:
[...]
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.
Apr 23 '06 #6

P: n/a
Ben C <sp******@spam.eggs> writes:
On 2006-04-22, Keith Thompson <ks***@mib.org> wrote:

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

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

--
Keith Thompson (The_Other_Keith) ks***@mib.org <http://www.ghoti.net/~kst>
San Diego Supercomputer Center <*> <http://users.sdsc.edu/~kst>
We must do something. This is something. Therefore, we must do this.
Apr 23 '06 #7

P: n/a
Keith Thompson wrote:
Ben C <sp******@spam.eggs> writes:
On 2006-04-22, Keith Thompson <ks***@mib.org> wrote:


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

--
Ian Collins.
Apr 23 '06 #8

P: n/a
Ben C wrote:

On 2006-04-22, Keith Thompson <ks***@mib.org> wrote:

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.


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;

--
pete
Apr 23 '06 #9

P: n/a
Keith Thompson wrote:
Ben C <sp******@spam.eggs> writes:
On 2006-04-22, Keith Thompson <ks***@mib.org> wrote: [...]
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.


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.
Apr 23 '06 #10

P: n/a
Ben C <sp******@spam.eggs> writes:
On 2006-04-22, Keith Thompson <ks***@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.


It makes it easy to find function definitions with "grep", which
is occasionally useful.
--
"I don't have C&V for that handy, but I've got Dan Pop."
--E. Gibbons
Apr 23 '06 #11

P: n/a
Edward Gregor wrote:
struct region {
int left, right;
int top, bottom;
};


Where's it's unit tests?

--
Phlip
http://www.greencheese.org/ZeekLand <-- NOT a blog!!!
Apr 23 '06 #12

P: n/a
Edward Gregor said:
Diomidis Spinellis wrote:
Edward Gregor wrote:
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?


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.............
--
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)
Apr 23 '06 #13

P: n/a
Richard Heathfield wrote:
Edward Gregor said:
Diomidis Spinellis wrote:
Edward Gregor wrote:
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?

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:


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
Apr 23 '06 #14

P: n/a
Ben Pfaff said:
Ben C <sp******@spam.eggs> writes:
On 2006-04-22, Keith Thompson <ks***@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.


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
"Usenet is a strange place" - dmr 29/7/1999
http://www.cpax.org.uk
email: rjh at above domain (but drop the www, obviously)
Apr 23 '06 #15

P: n/a
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 */

--
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)
Apr 23 '06 #16

P: n/a

Richard Heathfield wrote:
Edward Gregor said:
Sorry but what do you mean by inclusive and exclusive limits?
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

Apr 23 '06 #17

P: n/a

"Keith Thompson" <ks***@mib.org> wrote in message
news:ln************@nuthaus.mib.org...
Ben C <sp******@spam.eggs> writes:
On 2006-04-22, Keith Thompson <ks***@mib.org> wrote:

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


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

Apr 23 '06 #18

P: n/a

Ben Pfaff wrote:
Ben C <sp******@spam.eggs> writes:
On 2006-04-22, Keith Thompson <ks***@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.


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.

Apr 23 '06 #19

P: n/a
On 2006-04-23, Keith Thompson <ks***@mib.org> wrote:
Ben C <sp******@spam.eggs> writes:
On 2006-04-22, Keith Thompson <ks***@mib.org> wrote: [...]
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;


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!
Apr 23 '06 #20

P: n/a
In article <Os*******************@newsb.telia.net>,
Edward Gregor <ed****@hotmail.com> wrote:
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?


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

Apr 23 '06 #21

P: n/a
Edward Gregor wrote:
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
Apr 23 '06 #22

P: n/a
Richard Heathfield <in*****@invalid.invalid> writes:
Ben Pfaff said:
Ben C <sp******@spam.eggs> writes:
On 2006-04-22, Keith Thompson <ks***@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.


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

P: n/a
Ben C wrote:
On 2006-04-22, Keith Thompson <ks***@mib.org> wrote:
Diomidis Spinellis <dd*@aueb.gr> writes:
[...]
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.
Apr 24 '06 #24

This discussion thread is closed

Replies have been disabled for this discussion.