429,615 Members | 1,961 Online
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
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 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 San Diego Supercomputer Center <*> 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 wrote: Diomidis Spinellis 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 writes: On 2006-04-22, Keith Thompson 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 San Diego Supercomputer Center <*> We must do something. This is something. Therefore, we must do this. Apr 23 '06 #7

 P: n/a Keith Thompson wrote: Ben C writes:On 2006-04-22, Keith Thompson wrote: [...]Another style point: I always use braces on conditional statements,even when there's only one statement being controlled. For example, Iwrite 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 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 writes: On 2006-04-22, Keith Thompson 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 writes: On 2006-04-22, Keith Thompson 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 writes: On 2006-04-22, Keith Thompson 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" wrote in message news:ln************@nuthaus.mib.org... Ben C writes: On 2006-04-22, Keith Thompson 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 writes: On 2006-04-22, Keith Thompson 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 wrote: Ben C writes: On 2006-04-22, Keith Thompson 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 , 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! 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 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 writes: Ben Pfaff said: Ben C writes: On 2006-04-22, Keith Thompson 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 wrote: Diomidis Spinellis 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.