But is it beautiful code?
in [General Programming]
|
Prev: Sale Price Timberland Watches Casbah Replica - Timberland Minimum Price Wholesale
Next: ANN: Seed7 Release 2008-04-20
From: Richard Harter on 20 Apr 2008 15:11 A while back I posted a request for a dynamic storage allocation package similar to the one I posted in 1990. What I wanted was a more modern package with the same general requirements as the original. After checking around I found sundry good stuff but nothing like what I wanted. Ergo, the thing to do was to write it myself. The 1990 attracted the attention of a prolix personage who pronounced it beautiful code, mostly, I gather because it almost every line was commented. It may well have been beautiful - that I leave to others to judge - but the code was a trifle archaic and had a number of flaws, or at least that is my current opinion. As I say, I wrote a new version, reusing the old style of commenting and variable naming. The documentation may best be described as obsessive-compulsive. The new code style can be found at http://home.tiac.net/~cri_a/source_code/getspace/. There are five files to be found there, getspace.c, getspace.h, trace.c, trace.h, and readme.txt. (I haven't zipped it yet.) The code has been tested and is free of known bugs. However the full blown test suite is not yet complete, so its status is early beta. With that warning, everybody is free to use it and play with it as they choose. The license is non-viral. I believe, perhaps without good reason, that it is a good package and a much better package than its putative ancestor. Be all of that as it may, the real question is: Is it beautiful code? Richard Harter, cri(a)tiac.net http://home.tiac.net/~cri, http://www.varinoma.com Save the Earth now!! It's the only planet with chocolate.
From: Steve O'Hara-Smith on 21 Apr 2008 05:46 On Sun, 20 Apr 2008 19:11:24 GMT cri(a)tiac.net (Richard Harter) wrote: > I believe, perhaps without good reason, that it is a good package > and a much better package than its putative ancestor. Be all of > that as it may, the real question is: Is it beautiful code? Well it has inconsistent brace layouts in use (you used an opening brace for an if on it's own line once). Personally I really don't like if and for bodies on the same line as the condition it makes them hard to spot. For my taste there's too little whitespace and finally the arguments comments don't seem to line up properly. But hey, beauty is in the eye of the beholder. -- C:>WIN | Directable Mirror Arrays The computer obeys and wins. | A better way to focus the sun You lose and Bill collects. | licences available see | http://www.sohara.org/
From: Ed Prochak on 21 Apr 2008 13:08
On Apr 20, 3:11 pm, c...(a)tiac.net (Richard Harter) wrote: > A while back I posted a request for a dynamic storage allocation > package similar to the one I posted in 1990. What I wanted was a > more modern package with the same general requirements as the > original. After checking around I found sundry good stuff but > nothing like what I wanted. Ergo, the thing to do was to write > it myself. [] > As I say, I wrote a new version, reusing the old style of > commenting and variable naming. The documentation may best be > described as obsessive-compulsive. The new code style can be > found athttp://home.tiac.net/~cri_a/source_code/getspace/. > There are five files to be found there, getspace.c, getspace.h, > trace.c, trace.h, and readme.txt. (I haven't zipped it yet.) [] > I believe, perhaps without good reason, that it is a good package > and a much better package than its putative ancestor. Be all of > that as it may, the real question is: Is it beautiful code? > It does seem a little excessive on the comments. I honestly never cared for comments that simply rephrase the code, for example from your code: SDATA { /* Small bins search data */ int mark; /* Mark/unmarked flag */ size_t parent; /* Parent in sdata tree */ size_t right; /* Right child, if any */ size_t up; /* Where to search up */ }; These comments all repeat what is easily assumed from the field name, except maybe the up field. (even with the comment it is not clear to me what UP is really for). Elsewhere in the code, the comments are also redundant, for example in getspace(): trace("getspace"); /* Calling sequence trace */ if (notinit) initspace(); /* Initialize if needful */ rsz = sz_round_up_size (size); /* Round up, allow check chars */ All of those comments repeat the meaning of the code. Poor style IMO. FYI, I am always concerned when C programmers define TRUE and FALSE defines. In your favor, you only use them for return values (not for logic thank goodness!). But I would have to ask: why not some other words like PASS and FAIL? TRUE and FALSE are such loaded names. There is some attempt to get away from hard coded constants via defines, but then there are slip ups, for example: #define GETHASH(hash,address) \ do { hash = (size_t)address; hash -= (hash>>5); hash &=HMSK; } while(0) Why is HMSK a define but not the 5?? and on that note, why in the middle of initializing lots of variables to zero is the sequence starting at 2? static unsigned long ntotal = 0; /* Total number of nodes */ static unsigned long seqno = 2; /* Allocator sequence number */ static unsigned long maxstg = 0; /* Maximum space allocated */ static unsigned long curstg = 0; /* Current space allocated */ static unsigned long totstg = 0; /* Total storage managed */ (I would not have guessed that maxstg means max space. But you seemed to be thinking SToraGe, an unusual abbreviation. So the comments here helped.) I would expect notinit to be set in the cfgspace() routine. Since it is only checked there, I might have been more inclined to make it local to the initspace() routine and change initspace to return the inititalization status. Wait. I see you use notinit for other checks. Still I'm not sure I like this shared status, but on the good side, it is limited to the package. (Why do you need to initialize all over the place??) (this one is minor) In the header for getspace(), I would have preferred a description of the returned value. I can read the code to determine the return data type. Instead you have the return protocol in the function description. To make this better, I would just leave out the Return type line. And in getespace() I see that maxstg is not really an upper limit. So it isn't quite clear why you need both that and currstg. (I haven't delved into the logic, so you may have good reasons.) I merely glanced at the other routines. Some of the inline comments are good. Overall, it is reminiscent of an assembly program, so actually very appropriate for C (the glorified assembler programming language). Ed That's my opinion and I'm sticking to it! |