You are here

Code quality, NIH, golden code, and module ratings

Submitted by Morbus Iff on Fri, 2008-04-04 08:43

I'm pretty critical about code quality, be it for my own projects or those of others, and have become known as giving harsh code reviews for the Drupal community. Hearing "ask Morbus, but be prepared to cry" pleases me, and I have little impetus to stop doing things I enjoy, regardless of what people think. Recently, someone suggested the reason my reviews seem "assholish" is because I state "you" a lot. I agree with this, I think, and have made an attempt to lessen my usage, though not my anality.

I define code quality via a number of different hard-to-describe metrics. In the case of Drupal, following the code style guidelines is certainly key, but also following Drupal core patterns as well - doing things "like core" is pretty high up there. This could be as simple as using the same documentation string for Drupal hooks, using the same pattern for the null option of a select box, et cetera. Using coder.module (a Drupal lint) to correct 90% of your mistakes should be required and, sometime in the future, tests from something like SimpleTest should be too.

I also think that, if you're going to "take over" a module, or otherwise release one, it should make an attempt at meeting the highest level of code quality. One could argue that "release early, release often" dissuades us from this, but I disagree: there's a difference between "doing it right" and "continuing to suck". I have no problem with weekly releases if you make an attempt to maintain or improve code quality. Hearing "A lot of strings have been lingering in incorrect forms ... as I try to prevent translations from breaking" isn't a legitimate excuse: Drupal core doesn't care about backwards compatibility, and it highlights your own inability to spell-, grammar-, or read-aloud check your strings (while also calling into question the quality of translators who have converted your broken documentation without complaint). I didn't release "my" version of Case Tracker until I considered it of high quality, and I made sure to provide explanation for what I was doing.

Since I've stopped working on Case Tracker, the CHANGELOG.txt hasn't been updated but, more importantly, it hasn't been removed either. This is just the first of a dozen "heading back to crap" problems that Case Tracker is suffering under, as I watch the new maintainer commit style error after style error. I don't give two shits if you're a certain type of programmer, or if you've coded this way "for years": if you're committing a Drupal contribution, you should follow the goals and styles of the parent project. Anything else is utter failure. If you don't want to, get the hell out of contrib, because it's already a sodding mess and you'll only make it worse. Requiring developers to read even the most basic of code practice books should be a guideline for access into any repository. "What is the name of the checklist on page 579 of Code Complete First Edition?", and other "you must have the book" tests, would be awesome.

Which all leads us into the blanket response to any criticism of code: "Where's the patch?". The overuse of the "code is golden, talk is not" ideology is as frustrating as anything else. I have no problem with contributing patches to Drupal modules which have made an attempt at code quality. There's a difference in "your code is good, but this needs fixing" vs. "holy crap, everything about this module is wrong". If you make an attempt to write good code, I'll put forth the effort to make it even better. If you don't, then I'm not going to waste my time improving your code: it just needs to start over. Make a new file and port things carefully, rewriting and organizing them properly. Or, do it in the same file and separate the good stuff from the bad by gigantic swathes of whitespace. Until you make the effort to show you care about your code and the parent project it belongs to, I'm not going to either.

Quality is one of the prime reasons I wrote an (unreleased) Achievements module as opposed to installing User Points. Ignoring the different approaches of "achievements" vs. "accumulate[d] points" (which I'll probably touch on in a later post), the 2.0 version of User Points failed at nearly everything. Besides showing, to me, a lack of quality, it actually didn't work properly, further exasperated because it shipped with a number of modules that were never maintained. I sent an email or two stating my dislike, and marveled privately about how so much crap could make it into a "2.0" release, especially one that was proudly presented and ballyhooed at various Drupal conventions. This is why I don't mind complaining about it publicly - it positions itself as something great, but I've yet to see any indication of that. I don't think I'd be as aggressive if it were a new module or developer innocently asking for help - there's no marketing, ego, or pride to provoke my ire.

Inevitably, I got into a chat about it on IRC, and that ever-nefarious "patch?" lament appeared. To me, User Points 2.0 was an example of wrong from the ground up, with duplicate data in the database, a UI that didn't work (functionally or architecturally), shipped modules that no developer cared about anymore, and crazy code patterns that drove me up a wall. The same conversation promised a new developer had taken up the reins of 3.0 with lots of rewrites, and I should check it out. Hating duplicate functionality and the whole "Not Invented Here" problem that often plagues plugin-driven development, I told myself that I would.

I'm embarking on such a journey now, as I find a need for my Achievements module once again. And, still, as I look at the very first lines of User Points 3.0, I find myself clucking disapprovingly. I don't see much improvement but, certainly, it requires more effort to truly make the decision (to their credit, they did remove all the unmaintained contributed modules). For what it's worth, my first 15 minutes of discontent equate to:

Abuse of constants: User Points defines its permissions and other strings inside constants. This isn't only wrong (Drupal core doesn't do such a thing) but serves no discernible purpose besides being an additional layer of abstraction - perhaps the developer's IDE supports constant completion? If you're not manually and anally testing every line of code (which would root out misspellings or typos, one of a few rationales for code completion in an IDE), then your module isn't up to par. Gladly, SimpleTest integration in the future will indicate more obvious dedication to testing and non-laziness but, of course, that presumes properly written and maintained tests, which is probably too much to ask and could lull one into a false sense of complacency.

String retardation: Administration permissions in Drupal core are always in the form of "administer something", but User Points happily uses "admin userpoints". It also doesn't know what the hell it's called: the module's page has "User Points", but usage within the code indicate it could be "Userpoints" or "userpoints" and that it could integrate with "e-commerce" or "ecommerce". These are all things I consider easy and obvious indications of quality control - most module interaction is only ever to an end-user, and if you can't perfect the most visible output of your effort, then the actual code isn't getting any love either.

At a 15 minute glance, if someone yelled out "patch?", I'd decline. This is a module that is showing the same problems I lamented privately about for 2.0, and just needs to spend an entire release cycle worrying about quality not functionality. With that said, note that this is, literally, 15 minutes of glancing at the source. I've yet to actually install the new 3.0 and there's still a good chance I'll find it usable enough to merge with my achievements code. "Not Invented Here" syndrome is a sign of laziness too but, as jsled remarked after reading an early draft of this entry, "at some point, learning how to use a tool is more costly than just writing a special-purpose tool".

One suggestion for improving module visibility in Drupal is to offer module ratings. I wouldn't support this idea unless there were two distinct metrics: the end-user metric (does this module do what I want it to do?) and the developer metric (does this module's code live up to the code we ship in Drupal core?). To me, a module's quality equates to a combination of both, and any rating system that attempts to merge the two, or leaves one out, isn't adequately asserting overall health.

Tags:

Comments

My biggest concern with Drupal and my top wish for future versions is to maintain better compatibility with existing modules. Every new version breaks many of the contributed modules I use. I still haven't been able to upgrade to 6.x since many modules I use, including CCK, Views, Buddy List, MySite, and User Points still haven't been updated and still aren't fully compatible.

In the future, please make a better effort to maintain better compatibility with older modules or work with module developers to ensure that they have compatible versions of their modules when the next version of Drupal is released?

My experience with WordPress 2.5 was the exact opposite. None of my themes or plugins broke and the upgrade was quick and painless. I want Drupal upgrades to be just as easy.

Perhaps people confuse "a lack of functionality" with "a lack of quality"?

Personally I found this description of Release early, release often enlightening. Note the second bullet, about shame. This suggests increasing the visibility of code quality - or lack thereof - will improve the effectiveness of the RERO principle.

@Morbus: I've been annoyed by the lack of code quality (perhaps code care is a better description?) too, endlessly. I think I'm going to reference this post quite a lot in the future. Thanks for this writeup!

Thanks for this write up on code quality. At the Open Source Business Conference last week I saw some results that indicated that more people were using open source because they believe the code quality is higher. This is a great start for explaining why using Drupal, and in particular it's quality code is better than many proprietary systems.

IMHO detailed code reviews would be excellent. They give credit where credit's due. They can inspire developers to take more care. And, possibly most importantly, they're highly valuable to people (relatively) new to Drupal development. Your review of linkdsb, for example, taught me a bunch of things on how it's done and how core should be an inspiration and starting point.

The rails community has therailsway.com, a code review blog that does just that. It's lead by two leading rails developers and is an excellent way to learn...

First, kudos to morbus on the blog post. I think that with (Drupal's) success will come higher and broader public scrutiny (which will by default include increasing examination of quality), and with continued code growth will come higher complexity, and potentially, fragility. Code quality, and a more rigorous testing infrastructure are going to need to become a serious focus of the Drupal community. I know Dries believes it; we hope the entire community makes it a priority.

Second, jpoesen's idea has merit. If others agree, too, maybe this is a thread worth starting on g.d.o. Any takers?

I sometimes feel like I'm stepping out-of-line when pointing out some of the things you mentioned here to seasoned module developers, so I rarely do it. After all, they've been developing longer than I. They've been involved with Drupal longer than I. They should know better than I what is correct and acceptable and what is not. Am I being the "snot-nosed, know-it-all kid" (in experience, not age) when I point out the Drupal coding guidelines, or the discussions on the development mail list where these decisions are made? Don't these people already know all this stuff and where to find them if they need a refresher? I know I frequently need reminding, and constantly return to the source to verify or disprove what I think is correct.

Might I also add that following the guidelines not only helps with quality, but it also helps with readability for those times when "patch?" is uttered? If I feel like I am going to have to spend a lot more time with reading through the code in order to make a change or three (because of the lack of coding style or following the guidelines), I am less likely to go through the effort, especially if I feel like my effort won't be accepted or maintained.

I have heard it said that working in contrib is a way to be involved with Drupal without having to comply with the coding guidelines. For me, however, it's the starting point of what my code should look like and strive to achieve.

IMO we have to move to a situation where contrib modules get the same treatment as core patches. Lots more eyes on the issue queues for 'big' modules and more reviews like that webform one. 'Not invented here' is causing major issues at the moment (that will be ameliorated, but not solved, by module ratings etc), and a more open, collaborative, and brutal approach to contrib modules would help all of us.

Nice writeup Morbus! I'm not writing bad code (nor good code) since I'm not a coder myself, but I understand the importance for the community. Too often new users will go and download whatever module says it does what they need only to find it falls short and causes all kinds of unforeseen problems and end up with a mess of a site.

Here are a few specific things that help (or could if implemented:)

Always look at the issue queue to get an idea of the state of the module. See how recent the activity is on it and if there seem to be any show stopper issues.

Have another developer review the code and test the functionality before using the module

As someone who has taken over a module that is a mess sometime you leave code you don't like and make changes slowly. I have taken over 4 modules in the last 6 weeks. There is stuff in them that I don't like but leave because time is limited and the best way to break things is to make massive changes all at once and then you get the joy of tiring to figure out what you broke. Not everyone agrees with me and there are times I think maybe I should just dump the project and do a version 2 and start from scratch.

As for coding standards the first thing I do when taking over a module is go thought it with coder to get as many of the 90% things fixed.

Note on simpletest. Show me a good quick start guild for simpletest. I have been looking I want to add tests but it seems to be worse then learning Drupal core to figure out. That will change over time and I look forward to figuring out how to use it.

I stumbled upon this article while searching for an "achievements modules". I love the comments about the coding practices (or lack thereof) and found myself nodding along while reading. I hate to disparage the effort, but I have rejected using many of the high-profile third-party modules for this reason.

Anyway, back to achievements. My custom Drupal application (still in development and not yet ready for public consumption) uses a fake monetary system. I want achievements tied to the accomplishment of certain milestones (through deliberate or incidental action) and not merely by the accumulation of user points (or monetary funds in my case.) My thought is rewarding users with avatar badges when achievements are made, which would be proudly displayed on their profile page.

In any event whatever become of your achievements module? I'm not necessarily looking to acquire it, but I would be interested in reading your expanded thoughts on the subject.

My achievements.module would do what you're looking for. It's pretty much "done" - the only thing stopping its release was actually creating more achievements for all the default functionality. E-mail me at morbus@disobey.com and I'll hook you up with a copy.

Why don't you give examples of how it should be done? I was interested in using userspoints module(s) for my new project. I'm a novice programmer and so wouldn't know the difference between good and bad coding practices, only perhaps as a user, ie it works or it doesn't.

Why don't you release your Achievements module, even without the extended functionality, or at least make it available of your site?

It is available, to anyone who currently asks (as have a few people in this thread). I don't release things, however, until I can support them or am proud of them. There's plenty of shitty code in the world. I attempt to only infect things positively.

@Morbus: old post, but so, whats your take (with this kind of thing in mind) for the future of Drupal 8 and beyond considering the move to distributed version control, will "patch?" turn into "clone?" ? Do you think code quality and care is likely to improve? I'm interested to see if you think git will have any effect at all on code quality.