About two weeks ago I added the postfixop check to krazy. This checks for postfix uses of ++/– usage. Initially I constrained the check to for-loops. However this lead to a huge amount of issues because also postfix usage of ++/– operators on simple types where marked as an issue. After some discussion with Eckhart Woerner and Allen Winterz I relaxed the check more so that usage on simple types is also not taken in account anymore.
Although postfix usage on simple types won’t gain you any/much I still think it’s a good thing to consequently use prefix wherever appropriate. However that has more a cosmetic reason (which I why I took it out actually) then a perfomance reason. Consider for example the next piece of code (and yes I’ve seen something like this somewhere in the KDE codebase):
QList::Iterator it = list.begin();
for( int i = someNumber(); it != list.end(); ++it, i++ )
{
...
}
I realize that it’s partly a matter of taste if you like this kind of code snippets in your code base or not. However for people who never have had some in depth programming course it might look like if it is arbitrary what you choose.
Another thing I’ve been thinking about last weeks is "What is the importance of cosmetic changes in relation to code quality". When browsing through the source of KMail while fixing issues reported by krazy I see a lot of things which make me cry. For example defining variables which are used only ~25 lines later or in 3 different context. These kind of things make my fingers itch to rewrite such parts. Well, don’t be scared, I didn’t….. if it ain’t broken don’t fix it, right? But I still can’t find a good answer on the question: is that sort of code broken? In some sense it is in my opinion. When I as a newcomer to the KMail code look into some sections I really have to read that twice or sometimes even more times to get some vague idea what is happening over there. I think that is something that can be avoided (to some extend of course, as not everything in life is as easy as we would like it to be). All this made me think that it would be a nice to have something in addition to the normal krazy checks which checks for situations where some cosmetic change might improve the readability (and therefore the quality?) of the code.
Well, leave me a comment if you have some noteworthy ideas about these topics.
p.s. KMail only served as example here, I guess there are more places which have similar issues. It’s just that I was browsing through KMails code while these thoughts came up.
Another problem I see is classes with a couple of thousand lines of code and 100 – 300 methods. Working with code like this feels like stitching using a tangled ball of whool: You want to turn the ball of whool into something more useful but instead you find yourself fighting with the tangles.
Likewise, the code of such classes is tangled by unneccessary interdependencies between these methods, making it hard to add or improve features. The problem is that such big classes provide many features at once, but the lack of grouping and encapsulation blurres the borders between them.
Huge classes like this can be found in very strong areas of KDE like the khtml and kate parts.
I think it is broken if it is hard to read as it means it isn’t easily maintainable and the whole “many eyes..” thing goes out the window if only a few eyes can read it.
Readability is the most important attribute of code. If it isn’t readable it is broken
> Readability is the most important attribute of code. If it isn’t readable it is broken
Second that. And it’s the code itself that is readable or not. not the formatting.
Mike
YJ1q6v xcbrbdcxxahz, [url=http://qcvbdmgushmj.com/]qcvbdmgushmj[/url], [link=http://bvrvlnatrjjv.com/]bvrvlnatrjjv[/link], http://eqvhqlxbplrv.com/
Well that is exactly my point. Using consistently ++foo will ensure that you always have the fastest option plus (and that is more a matter of taste) doesn’t make it look like if it’s arbitrary what you choose.
Excuse me, but this
QList::Iterator it = list.begin();
for( int i = someNumber(); it != list.end(); ++it, i++ )
{
…
}
is equivalent to
QList::Iterator it = list.begin();
for( int i = someNumber(); it != list.end(); ++it, ++i )
{
…
}
.
Because in almost all cases foo++ is as fast as ++foo but in some rare cases ++foo is faster one should prefer ++foo.
y5tXZv jytbgrhvaqci, [url=http://deqfhvemizhp.com/]deqfhvemizhp[/url], [link=http://hpqrnnqaspzj.com/]hpqrnnqaspzj[/link], http://xeuqggfeknlr.com/
zpJqZV oavcuvflaayd, [url=http://sipzqoadzwsb.com/]sipzqoadzwsb[/url], [link=http://khoxkmnvcyvw.com/]khoxkmnvcyvw[/link], http://eyzogdjxfqtz.com/
Mark, the main point of this kind of checks to me is not perse doing micro optimizations. My main concern is that using inconsistent coding style, bad coding practices, using constructs for task that they where not defined for, etc. make your code harder to read and harder to understand what is happening. And in some sense this breaks your code I think. When in the end only a handful of people are able to understand what is happening, then you have some kind of problem with regard to maintainability of your code.
And my suggestion was/is to not add these kind of checks to krazy directly (as fixing such issues is in a lot of cases certainly not a trivial task) but to have some separate section/site/whatever that reports issues and suggests alternatives.
I have to say I find it a bad idea to add such micro-optimization checks to EBN. For the most part they are completely irrelevant for real life performance, and just tend to bloat EBN and reduce its relevancy.
Especially inexperienced programmers may take these hints too seriously and forget good program design over optimizations.
Man this code is JUST BAD. And it is not matter of taste.
I’d certainly consider convoluted code broken and worthy of a bug report, although priority would depend on the prominence and importance. Everyone’s aware of user-centred design in UIs these days, but that also applies to code, for which the programmers themselves are users. Bad code is just like a bad UI — the program way work, but it’s still broken.
bW4Chq cboxvmvgqams, [url=http://jyehgripbehg.com/]jyehgripbehg[/url], [link=http://ltsmuuhvznaw.com/]ltsmuuhvznaw[/link], http://igizswenpobo.com/
Although postfix usage on simple types won’t gain you any/much I still think it’s a good thing to consequently use prefix wherever appropriate.
Could you explain what you mean here by ‘gain you much’? Do you mean readability, portability or run time speed – it isn’t clear to me. I personally always use ‘i++’ in loops as I think it better reflects what the code is doing and is slightly easier to read – ie you increment to loop variable after running the body of the loop, even if in this case ‘++i’ will always work exactly the same way.
Hi Richard, I was mainly referring to run time speed here. I can agree on the fact that i++ reflects a bit better on what is happening, oth it still looks awkward when in the cases that ++foo is more effictive one uses ++foo in stead of foo++ and certainly when the two different ways are used in one for loop.
So consistently using ++foo in for loop will gain you readability mainly.
Isn’t it better to write something like this?
QList::Iterator it = list.begin();
QList::Iterator it_end = list.end();
for( int i = someNumber(); it != it_end; ++it, ++i )
{
…
}
No, because it has two issues:
1) it_end is not const, so it can be modified anywhere in the code, and
2) you pollute the scope around the for loop with yet another variable which is only ment to be valid inside the scope of the for loop
Zcbx9R wznfkpxvdher, [url=http://nplzwmmptqso.com/]nplzwmmptqso[/url], [link=http://ucxaqlfykrwz.com/]ucxaqlfykrwz[/link], http://uhoggghdtlfn.com/