While reviewing a co-worker's code, I came across some spelling mistakes in function names and also grammatical errors like doesUserHasPermission() instead of doesUserHavePermission() in function and variable names.
Should I point these out to him or am I being too pedantic by noticing these?
Code with spelling and grammar errors is unmaintainable.
People won't remember the bad grammar, so they'll try to call the function as it should have been written, and that's how bugs happen.
You can't grep for something in the code if you don't know how it's spelled.
Most people who make grammar/spellings do so inconsistently, so they'll introduce many bugs with mismatched naming. This is particularly problematic in languages that don't require variables to be explicitly declared before use, because you can introduce a new spelling and your code won't come to a grinding halt to let you know you screwed up.
Correcting these problems is not pedantic, nor is it necessitated primarily by others' opinions of one's intelligence, literacy, etc (though that's a big side-effect); it is about writing quality, maintainable code.
Yes definitely. It's easier to remember the name if it's grammatically correct. Trying to remember the name and grammar mistakes is another thing entirely.
Don't point them out as defects in a formal code review. Instead, mark up a listing and talk with him/her PRIVATELY about them. Be as diplomatic as possible about it, just "Hey, something I noticed, and I've run into people who REALLY look down on this kind of thing, they think it makes the programmer look careless and sloppy."
If this is code a customer is going to see, it absolutely MUST be corrected. Like it or not, it DOES reflect on your company's reputation.
For the example you gave, I suspect it started out as UserHasPermission, and someone else told him that local practice was doesUserBlahBlah() rather than UserBlahBlah(), and he just overlooked the grammar change.
Change it yourself.
Hopefully you're in an environment where code "ownership" is not an issue. If you have access to the project in source control, just go in and fix it yourself. If you see a particular coworker making the same type of grammar or spelling errors consistently, you might want to point it out, but that will depend on your relationship, whether the person is a native English speaker, and their general receptiveness. But whether you ever decide to do that or not, just quietly go and make the fix. I do this all the time, if I see a typo, especially in a method signature or public property, I just fix it. Occasionally I can't even resist the temptation to fix a typo in a comment, but that's just me :)
I guess its worth mentioning here that the HTTP referrer header in the HTTP protocol was misspelt as "referer" (and we have to live with it/we have learnt to live with it .) :)
I'm a developer whose native language isn't English, it's Dutch actually, and wouldn't mind at all if someone would point me a grammar or spelling mistake. In that way I can constantly keep improving my English. And it is certainly not difficult to correct all the mistakes in all of your source code. A simple Perl script can easily be written to loop thru all files in a folder. Perhaps even it can be done with sed? I don't know.
So I would certainly point out grammar or spelling mistakes in someone else's code, but only if I'm absolutely sure wether it is correct what I'm saying.
I agree with other answers saying that code with grammar mistakes is unmaintainable.
I also want to add a few things:
I would recommend using an IDE with built-in spellchecker. IntelliJ Idea does a wonderful job for Java programs. There are many embarassing typos it catches, not only in names of functions, but in e.g. exception messages the user gets to see. A program that produces messages full of typos does not inspire much confidence.
I do it only if
Just as a side note, if your function names are long enough to have grammar, they're probably too long. In the example given, I would call the function userHasPermission and move the "grammar" into your code, something like this:
if userHasPermission() ...
This also happens A LOT in my project (being populated by natively Hebrew, Russian or Arabic speaking people), but even to a higher level - often I see code that uses some obscure terminology that happens to be what the dictionary produced as the translation for what the author had in mind, and it has nothing to do with the they meant...
Personally, when it happens so frequently and by so many team members that could have wrote the code even before I joined the project, I tend to ignore it, because it just won't matter.
However, if I'm committing some work in the same file as code or comments that have been written long ago and they have typos in the, I will correct them just because it's not too much work.
Golden Rule Applies
Do unto others as you would have them do unto you.
I want others to have my back on this kind of thing, so I help others. Being gracious and supportive can go a long way in your favor.
This is a minor mistake in code, but is a mistake. Treat it like any other mistake that you find. My policy is always to assume that my co workers are competent and treat them that way until they prove otherwise.
If it's a single mistake I might just fix it and check it in. If it's a pattern I might start getting that co worker to review those fixes. Let them know that you think they're a good coder, but that this is something that would be good to improve on. I don't think I'd ever make a big deal about something like this though.
As long as you don't treat it like it's a big issue it should be easy to put that co worker in a position where they can improve without putting ego on the line.
Sure point it out, but don't waste your time checking for spelling mistakes. Use a tool to automate this on your CI. On .net fxCop can do this...
It depends largely on what the mistakes are, how common and how bad they are, and whether it's actually a bona fide mistake or just not how you'd word it.
I personally can't stand it when some idiot drags a 5 minute code review out to half an hour because he wants everything renamed to how he'd do it and all the comments reworded just because he likes sticking his oar in. A logging line that says "Loading data objects" does not need to be changed to "The data object loader component will now load the relevant data objects from the data object storage component".
/rant :)
As with many other good programming practices, the only objective, non-political, and effective way to implement a policy about spelling in programs is to automate it as part of the pre-commit process. Automation will save you from enormous amounts of grievance even if you have to write your own tool for the purpose.
