brooks@lll-crg.ARPA (Eugene D. Brooks III) (07/07/85)
> It is entirely possible to understand the semantics of the code fully and > still consider it unnecessarily obscure and hard to read. The mark of a > good programmer is that his code is *easy* to read, not just *possible* to > read. Using every obfuscatory feature of the language, perhaps in the > (oft-mistaken) belief that it improves efficiency, and then pleading that > "any experienced programmer should be able to understand that" is the mark > of someone who doesn't understand what this business is about. Not being a professinal progammer or even a computer scientist, perhaps my opinion that if((fptr = fopen(filename, "r")) == NULL) { fprintf(stderr, "Can't open %s for reading\n", filename); exit(1); } if perfectly clear to anyone who understands the C language and is not obfuscatory is misinformed. You can be assured that I will continue to use it and I will not hire any "professional programmers" who consider it bad programming practice. They may have a case with the employment office however, refusual to hire on the basis of their religion! Any C programmers who can't easily understand the above example simply aren't good enough to be working for me. When they aren't hired, or are fired, they will have no recourse. I do agree with the point that using subtle features of the C language which make even the most experienced C programmer stop and think for a while about what is being said is a bad programming practice. An example of such stuff would be if(a = b++ * --c) and I do remember a lot ot even more undesirable examples. I therefore agree with the point made above about writing super terse programs. The fopen case above simply isn't one of them.
henry@utzoo.UUCP (Henry Spencer) (07/09/85)
> ... perhaps my opinion that > > if((fptr = fopen(filename, "r")) == NULL) { > fprintf(stderr, "Can't open %s for reading\n", filename); > exit(1); > } > > if perfectly clear to anyone who understands the C language and is not > obfuscatory is misinformed. You can be assured that I will continue to > use it and I will not hire any "professional programmers" who consider > it bad programming practice... That's probably just as well, since they probably don't want to work for someone who thinks "readability" is a Boolean characteristic! I didn't say it wasn't readable, or that I couldn't read it; I said that it isn't *as* readable as it *could* *be*, and that this is undesirable. The degree of readability of "if ((foo=..." is a reasonable subject of debate, but I decline to back down on the general assertion that one mark of a professional is a genuine effort to *maximize* readability, not just to bring it above some minimum threshold. -- Henry Spencer @ U of Toronto Zoology {allegra,ihnp4,linus,decvax}!utzoo!henry
manis@ubc-cs.UUCP (Vince Manis) (07/10/85)
I must agree with Henry Spencer that readability is not a Boolean type (I doubt
it's even an ordered type!). However, the question of embedded assignment is
not entirely onesided. For some time, I have been using a construct which I
call a ``guarded command'' (and is indeed something vaguely like Dijkstra's
guarded commands):
if ((fp = fopen(...) != NULL) {
... Do something; fp does indeed have a value ...
}
The idea is that the if-statement protects the statements of the block. I
find this an extremely readable way of stating constraints about the validity
of variables over sequences of code.petera@hcrvax.UUCP (Smith) (07/11/85)
Concerning the expression if ((f = fopen(junk,"r")) == NULL) and the readability arguement. To say that composition of functions causes readability problems is a bit silly. We are forever composing functions in any language. Would the author of this complaint say that the code for expressions should be limited to an assignment and a mathmatical operation? But , that is composing two functions! To waste time worrying about such a trivial piece of code is silly when there are examples of code that are legitimately bad and whoose use should be avoided. Example: the use of pointers in 'tbl'. Peter Ashwood-Smith, Human Computing Resources, Toronto, Ontario.
mr@hou2h.UUCP (M.RINDSBERG) (07/12/85)
> I must agree with Henry Spencer that readability is not a Boolean type (I doubt > it's even an ordered type!). However, the question of embedded assignment is > not entirely onesided. For some time, I have been using a construct which I > call a ``guarded command'' (and is indeed something vaguely like Dijkstra's > guarded commands): > > if ((fp = fopen(...) != NULL) { ^ |--------- Missing close paren. This one sure is guarded, it wont even compile (>:) . > ... Do something; fp does indeed have a value ... > } > > The idea is that the if-statement protects the statements of the block. I > find this an extremely readable way of stating constraints about the validity > of variables over sequences of code. mark ..!hou2h!mr
lee@eel.UUCP (07/13/85)
The degree of readability of "if ((foo=..." is a reasonable subject of debate, but I decline to back down on the general assertion that one mark of a professional is a genuine effort to *maximize* readability, not just to bring it above some minimum threshold. As with any form of communication, there is no absolute scale on which to evaluate a speech, lecture, article or program as to its "readability", that is, the degree to which a person can absorb the information contained within it. People who teach writing and communication stress that the item MUST be designed with the expected audience in mind. An article written to explain FORTRAN to novice programmers will be inefficient (i.e. not maximally readable) to an experienced programmer. Likewise, programs "likely" to be read by expert programmers might well be more readable when "if ((fp=fopen..." is used even if for other programmers the opposite is true.
ludemann@ubc-cs.UUCP (Peter Ludemann) (07/13/85)
For those who think "if ((foo=fopen(filename,"r"))==NULL) { ... }"
is hard to understand, why not create a macro to handle this:
#define opentest(filename,mode,ptr) ((ptr=fopen(filename,mode)==NULL)
and then you can write "if (opentest(filename, "r", foo)) { ... }"
To understand this, you just have to refer to the usual 10 pages of inscrutable
#defines which come with the program -).
But I do think that the macro approach is slightly more readable.
It almost looks like a subroutine call with a "var" parameter.chris@umcp-cs.UUCP (Chris Torek) (07/16/85)
> ... [an appropriate macro] almost looks like a subroutine call with a > "var" parameter. Then there are those of us who feel that ``var'' is an abomination and that *all* calls should strictly be by value, and go out of our way to make fixed size arrays hide inside structures. Have you ever tried to understand some piece of code that had various calls, then (after long and painful thought) discovered that all those variables were being modified by some other routine (which might not even be in the same file!)? I think it makes code much more readable when some special mark precedes all parameters which are potentially modified. (In other words, it's not that I object to ``var'': I object to the callers not having to (re)state that the object is indeed modifiable.) -- In-Real-Life: Chris Torek, Univ of MD Comp Sci Dept (+1 301 454 4251) UUCP: seismo!umcp-cs!chris CSNet: chris@umcp-cs ARPA: chris@maryland
preece@ccvaxa.UUCP (07/16/85)
> For those who think "if ((foo=fopen(filename,"r"))==NULL) { ... }" > is hard to understand, why not create a macro to handle this: > #define opentest(filename,mode,ptr) ((ptr=fopen(filename,mode)==NULL) > and then you can write "if (opentest(filename, "r", foo)) { ... }" ---------- Unfortunately, the name 'opentest' doesn't imply that the variable named 'foo' has been set to the new fd and that the file is now open. It implies (to me, at least) a test of whether it is possible to open the file. Now, if you wanted to call it "TRY_TO_OPEN", I would be more likely to interpret the name correctly, but some people might interpret that name as a Boolean indicating whether or not to try to open the file. Naming is very tricky. Doing the operation is very clear. There's always room to argue about whether a particular expression is too complex. In this case I don't think anyone would have difficulty with the original form. The insightful programmer tries to notice and simplify expressions that are likely to be a problem. Arbitrary rules are unlikely to be satisfying. -- scott preece gould/csd - urbana ihnp4!uiucdcs!ccvaxa!preece
ludemann@ubc-cs.UUCP (Peter Ludemann) (07/20/85)
In article <2600006@ccvaxa> preece@ccvaxa.UUCP writes: > >> For those who think "if ((foo=fopen(filename,"r"))==NULL) { ... }" >> is hard to understand, why not create a macro to handle this: >> #define opentest(filename,mode,ptr) ((ptr=fopen(filename,mode)==NULL) >> and then you can write "if (opentest(filename, "r", foo)) { ... }" >---------- >Unfortunately, the name 'opentest' doesn't imply that the variable >named 'foo' has been set to the new fd and that the file is now open. >... Naming is very tricky. Doing the operation is very clear. If doing the operation is so much clearer, why do we bother with subroutines? After all, to figure out what the subroutine does, you've got to look through its code, but if the operation were done inline, there would be no problem :-) Perhaps, the solution to all this would be a procedure. Calling 'openfile(filename, &fileptr, "r")' gets 'fileptr' set appropriately. If the open fails a message goes to stderr and 'exit' is called. Of course, for some software (editors, for example), this is not desirable behavious, so something else is do We could go on arguing this forever. But, for the record, I consider assignments in 'if' statements dangerous because I expect 'if' statements to only do tests. Likewise function calls within 'if' statements where parameters get updated (like the macro I gave above). Likewise cryptic stuff like: while (*s++ = *t++); Use 'strcpy' instead! Trying for micro-efficiencies at the cost of obscure code is the sign of an immature programmer or a poor typist. Run a profiler on your programmer if you think it needs speeding up - you'll probably find something really gross that slipped by you while you were trying to construct an 'if' statement with three '++'s imbedded in it.
preece@ccvaxa.UUCP (08/05/85)
> If doing the operation is so much clearer, why do we bother with > subroutines? After all, to figure out what the subroutine does, you've > got to look through its code, but if the operation were done inline, > there would be no problem :-) ---------- Well, there's a qualitative difference between using a subroutine to replace a number of lines of code to perform a complex operation and using a macro with several arguments to replace a more straightforward construction. I don't use embedded assignments because they're more efficient (are they?), I use them because they make sense. The first time I saw them (18 years ago in Dartmouth BASIC) I recognized them as superior chunking agents. Remember chunking? Psychologists tell us that we build separate concepts up into more complex chunks so that we can comprehend more than seven (plus-or-minus two) concepts at a time. Things like embedded assignments and increment operators help [me, at least] build up a larger logical operation than would be possible otherwise. The ONLY thing I object to about them is that there ought to be a more obvious distinction between the assignment and comparison operation symbols. ---------- > Perhaps, the solution to all this would be a procedure. Calling > 'openfile(filename, &fileptr, "r")' gets 'fileptr' set appropriately. > If the open fails a message goes to stderr and 'exit' is called. ---------- We all have our preferences. You dislike embedded assignments, I dislike procedures changing their arguments. I'm currently working on a project that has a huge collection of macro definitions. Without them life would be impossible: they make it possible to do things neatly and transparently. They also make some kinds of problems very hard to find, at which point it becomes necessary to run cpp on the code and see what is really happening. Defining macro expansions and procedures so that they behave in a sensible, consistent, predictable fashion is very, very difficult. ---------- > But, for the record, I consider assignments in 'if' statements > dangerous because I expect 'if' statements to only do tests. ---------- Chacun a son gout. All that really matters are consistency and clarity. If you work within an orderly style and explain what you're doing, your successors will figure it out. ---------- > Trying for micro-efficiencies at the cost of obscure code is > the sign of an immature programmer or a poor typist. ---------- We just differ on what is obscure. I think that the dense but straightforward "if ( (foo=fopen(filename,"r")) == NULL) { ... }" IS clear and simple. I think that hiding it in a macro or breaking it into two lines makes it less clear and more obscure. There's nothing wrong with this kind of disagreement. I wouldn't say either approach indicates, by itself, immaturity or unprofessionalism. ---------- > Run a profiler on your programmer if you think it needs speeding up - > you'll probably find something really gross that slipped by you while > you were trying to construct an 'if' statement with three '++'s > imbedded in it. ---------- This is the absolute truth. If efficiency is the issue, always look for big gains first. Inefficiency is MUCH more likely to come from a broken algorithm than from careless code. On the other hand, DO go back over your 'finished' code and look to see that you've been consistent. The very best thing you can do for future maintenance programmers is to make sure you always do the same thing the same way. -- scott preece gould/csd - urbana ihnp4!uiucdcs!ccvaxa!preece