Newsgroup: comp.lang.c

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

Unless otherwise expressly stated, all original material on this page created by Diomidis Spinellis is licensed under a Creative Commons Attribution-Share Alike 3.0 Greece License.