On 10-1-2014 21:54, Tavmjong Bah wrote:
On Fri, 2014-01-10 at 21:00 +0100, Johan Engelen wrote:
Hi all, Lately, I've become obsessed with code quality. I've been reading Bjourne Stroustrup's "bible" on C++11, watched all videos of the "Going Native" conferences, started using clang, ..., and it has made me much more aware of the language's facilities for preventing bugs. I regularly browse over Inkscape's code and try to fix things I think can be improved (janitorial); our source is a pretty big mess I really do think Inkscape can use some love to improve logic and to decrease some amount of spaghetti.
When ever you find something janitorial... please send an email to the list. I am always eager to learn!
There are many many small things... Here an exercise. It's a change to buildtool, so it's not very important because we really should move to a unified system after the release.
http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/12913#buil...
The question: what do you think about the UniqueParams function? (it is great that Matt improved our btool, it's just a very recent and easy example with lots of improvements possible)
My comments, in the order in which I spotted them: - why is it virtual? no need for it, and I don't think it makes sense here either. - why start with capital? Our functions never start with caps, it makes it look like a constructor of a class (class types are with initial capital) - most variables declared way too early and thus with unnecessary large scope. (Matt does this a lot, a habit that stems from old compiler/language limitation and is really bad for our code (note: modern C also allows declaration of variables anywhere in function) - remove tab indentation !!! - source is passed by non-const reference, and this method modifies it. perhaps rename function to "makeUniqueList" or something that more clearly indicates that the argument will be modified. - important one: C++ provides std::set that does exactly what is needed here: keep track of a unique set of things. this function can be simplified a lot by just using std::set<string>. (know the C++ STL!) - "it++" in for statement, is not dramatic, just bad style (theoretically it++ is less efficient than ++it). - source +=std::string(" ") ---> source += " " No need to create a string object first, and then add that to string. More efficient to not "help the compiler" here, but performance is not the issue, more style/code clutter. - function could be declared const
And after rewriting the function using my comments, probably there are still things left to improve...
Ciao, Johan