Code cleanup and minor tweaks
Hi Campbell,
I just wanted to thank you for the recent improvement to our codebase, such as in revision 10359. Getting things fixed such as not passing 'false' in to pointers helps in many ways, including reducing the number of odd, hard to reproduce bugs.
I also just wanted to point out a few minor style points that could help. One is with the use of "NULL". That is more of a C macro that in C++ has be replaced with the simple use of "0". I've looked into this point with coworkers and others at various points, and can try to dig up more details if you need them.
The other point is in using whitespace. I realize that in your recent changes you have been preserving what already existed, but it is helpful if "cleanup" that touched lines left them with nicer spacing also.
We have some info on our coding style page, but the gist is to put spaces between things, but not in the middle of things. :-)
That is, space around keywords, operators, etc, but not between a function name and its opening paren.
For example, this line you touched could be improved by spacing it thusly
&&((l!=NULL&&!edge->isEnd(l->id))||l==NULL) && ((l != NULL && !edge->isEnd(l->id)) || l == NULL)
Suddenly that makes the logic and the referencing easier to read and follow. It also gives more visibility to areas that could be made clearer with more parenthesis:
&& (((l != NULL) && !edge->isEnd(l->id)) || (l == 0) )
if I compare those right on top of each other, the legibility change should show up:
&&((l!=NULL&&!edge->isEnd(l->id))||l==NULL) && (((l != NULL) && !edge->isEnd(l->id)) || (l == 0) )
Depending on the font, that might even reveal another problem. In general I strongly dislike single-letter literals (except for the common 'i' in for loops, etc). And even worse is the single letter variable 'l'. In fact, in the font I'm typing with at the moment I can not tell the difference between the lower-case-letter-el, the capital-letter-eye and the vertical-bar-pipe-| (lI|). What is worse is some fonts even make the numeral -one indistinguishable. But this is a side rant on a given bad variable. If you happen to be touching enough lines, consider giving it a better name.
Anyway, thanks for the help in this area and the opportunity for me to point out to some of the other newer people on the list what sort of tweaks can help our codebase be more legible and maintainable.
On Jun 25, 2011, at 3:22 PM, Jon Cruz wrote:
For example, this line you touched could be improved by spacing it thusly
&&((l!=NULL&&!edge->isEnd(l->id))||l==NULL) && ((l != NULL && !edge->isEnd(l->id)) || l == NULL)
Suddenly that makes the logic and the referencing easier to read and follow. It also gives more visibility to areas that could be made clearer with more parenthesis:
&& (((l != NULL) && !edge->isEnd(l->id)) || (l == 0) )
if I compare those right on top of each other, the legibility change should show up:
&&((l!=NULL&&!edge->isEnd(l->id))||l==NULL) && (((l != NULL) && !edge->isEnd(l->id)) || (l == 0) )
Or to take this refinement a bit further (as ScislaC commented in IRC), one could fold out the other NULL
&& (((l != NULL) && !edge->isEnd(l->id)) || (l == 0) ) && (((l != 0) && !edge->isEnd(l->id)) || (l == 0) )
Then again, the test against NULL/0 is not required in C/C++
&& ((l && !edge->isEnd(l->id)) || !l )
but I can't read that in this font, so I'll do a before and after with a new name:
&&((el!=NULL&&!edge->isEnd(el->id))||el==NULL) && ( (el && !edge->isEnd(el->id)) || !el )
Hi John, regarding use of NULL, I'm mostly a C developer so I didn't realize this though I did see it used in many other places
# grep -R NULL * | grep ".cpp:" | wc -l 8307
I was also tempted to do other cleanups but thought it best not to tread on anyones toes where style is concerned, especially since it was my first commit to the source code (and not cmake).
But agree with your suggested changes.
On Sun, Jun 26, 2011 at 12:47 AM, Jon Cruz <jon@...18...> wrote:
On Jun 25, 2011, at 3:22 PM, Jon Cruz wrote:
For example, this line you touched could be improved by spacing it thusly
&&((l!=NULL&&!edge->isEnd(l->id))||l==NULL) && ((l != NULL && !edge->isEnd(l->id)) || l == NULL)
Suddenly that makes the logic and the referencing easier to read and follow. It also gives more visibility to areas that could be made clearer with more parenthesis:
&& (((l != NULL) && !edge->isEnd(l->id)) || (l == 0) )
if I compare those right on top of each other, the legibility change should show up:
&&((l!=NULL&&!edge->isEnd(l->id))||l==NULL) && (((l != NULL) && !edge->isEnd(l->id)) || (l == 0) )
Or to take this refinement a bit further (as ScislaC commented in IRC), one could fold out the other NULL
&& (((l != NULL) && !edge->isEnd(l->id)) || (l == 0) ) && (((l != 0) && !edge->isEnd(l->id)) || (l == 0) )
Then again, the test against NULL/0 is not required in C/C++
&& ((l && !edge->isEnd(l->id)) || !l )
but I can't read that in this font, so I'll do a before and after with a new name:
&&((el!=NULL&&!edge->isEnd(el->id))||el==NULL) && ( (el && !edge->isEnd(el->id)) || !el )
All the data continuously generated in your IT infrastructure contains a definitive record of customers, application performance, security threats, fraudulent activity and more. Splunk takes this data and makes sense of it. Business sense. IT sense. Common sense.. http://p.sf.net/sfu/splunk-d2d-c1 _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
On 26-6-2011 0:22, Jon Cruz wrote:
One is with the use of "NULL". That is more of a C macro that in C++ has be replaced with the simple use of "0". I've looked into this point with coworkers and others at various points, and can try to dig up more details if you need them.
I think I like the use of "NULL" much more than "0", as it more clearly indicates the type.
Consider this code example of initializing variables: box->persp_href = 0; compared with box->persp_href = NULL;
I think the second version is clearer about what is being done. (the first looks like the number of refs is set to zero, instead of initializing a ref pointer
Ciao, Johan
On Sun, Jun 26, 2011 at 9:02 PM, Johan Engelen <jbc.engelen@...2592...> wrote:
On 26-6-2011 0:22, Jon Cruz wrote:
One is with the use of "NULL". That is more of a C macro that in C++ has be replaced with the simple use of "0". I've looked into this point with coworkers and others at various points, and can try to dig up more details if you need them.
I think I like the use of "NULL" much more than "0", as it more clearly indicates the type.
Consider this code example of initializing variables: box->persp_href = 0; compared with box->persp_href = NULL;
I think the second version is clearer about what is being done. (the first looks like the number of refs is set to zero, instead of initializing a ref pointer
Ciao, Johan
Ah, thought you were saying _not_ to use NULL in C++, agree with you its easier to understand. the source code checker 'sparse' can find these cases though its C only.
On Jun 26, 2011, at 8:38 PM, Campbell Barton wrote:
On Sun, Jun 26, 2011 at 9:02 PM, Johan Engelen <jbc.engelen@...2592...> wrote:
On 26-6-2011 0:22, Jon Cruz wrote:
One is with the use of "NULL". That is more of a C macro that in C++ has be replaced with the simple use of "0". I've looked into this point with coworkers and others at various points, and can try to dig up more details if you need them.
I think I like the use of "NULL" much more than "0", as it more clearly indicates the type.
Consider this code example of initializing variables: box->persp_href = 0; compared with box->persp_href = NULL;
I think the second version is clearer about what is being done. (the first looks like the number of refs is set to zero, instead of initializing a ref pointer
Ciao, Johan
Ah, thought you were saying _not_ to use NULL in C++, agree with you its easier to understand. the source code checker 'sparse' can find these cases though its C only.
What I've seen is that after a bit with either, the legibility is about as good. That is, when I've worked on code that used '0' instead of 'NULL', the '0' became more the readable one over time. It does get to be a bit subjective (although there are edge cases where NULL does not work as well in C++). I think much of preferring NULL comes down to "what you're used to".
Another problem with the legibility of using '0' instead of 'NULL' can come from those familiar only with C coding, and who therefore don't expect that '0' can be a valid pointer.
Now on the other hand Johan used an example that exposed a *different* problem with the code, IMHO. He cited a good case of potential confusion... however I would say that the problem is in the naming, not in the value being assigned. That is, if one is expecting the variable named "_href" to contain a count, then there is a naming problem. *If* something were to be a count, I would like to see a good self-documenting variable name such as "_hrefCount" or "_refCount". If we have clear names like that, then one reading the code would not expect "_href" to contain a count of anything, since it did not say it did.
participants (3)
-
Campbell Barton -
Johan Engelen -
Jon Cruz