Code Quality Issues
Posted: 07 Aug 2013, 12:23
I don't like this part, but apparently it has to be done. Our code base is starting to suffer from slowly creeping in code quality issues. This has been going on for quite a while (at least a year or so), without really becoming much of a problem. However I feel that we have reached a point now, where we either have to stop it or give up on stopping it.
There is no need for full scale code review and improvement (isn't that bad yet), but please keep the following points in mind when working on OpenMW:
1. Code Duplication
Avoid it. If you have the same code twice in multiple locations, that is a clear maintenance disadvantage. More work for people who need to change the code, more potential for creating bugs (by having the two copies getting out of sync) and generally needlessly large code. We are not some derpy proprietary software development company where the quality of your work is measured by the number of lines of code you produce. So please think twice before you use copy and paste.
2. Don't litter!
Seriously, guys! Clean up behind yourself. Least significant problem of the four listed here, but it aggravates me the most.
In the early days we had a developer who produced functional code in a decent quantity, but the files he touched always looked as if a bomb had exploded. Our situation is not that bad yet, but we are getting there (slowly).
Please remove unused code. Functions and local/member variables that are not in use anymore, but are not outcommented, are a real problem. They slow down people trying to read through the respective part of the code. Also, they sometimes produce warning messages, that I have to fix later (since we are aiming for warning-less code).
Even outcommented code is a problem. Because it blows up the source files unnecessarily and still somewhat slows down people trying to understand the code.
The only point where I have no problem with unused code is if that code is basically fine, but can't be used currently, because somewhere else something is broken. In this case however you should add a comment to the unused code that describes the problem.
For everything else we have git. If you are dealing with unused code that you might want to make use of again some day and you are worried, that it might get lost in the vast caverns of the repository, then just add a tag to it.
git also offers tools to detect messy code. git status before a commit is pretty much a must. But you might even consider doing a git diff after a particular long and complex coding session that is more likely to have unused leftovers.
3. Comments
If you have a look at our Ohloh Page, you will notice, that we don't have a lot of comments in the code. I am actually fine with that (they call me a bloody minimalist for a reason) and generally I don't give a rat's arse about this kind of metric.
However it does seem that we are missing important comments in a few specific areas.
If a function does something that is not obvious from its name and its general location in the system, then a short comment should be added in the header file.
If a function parameter has particular constraints or other oddities then a short comment should be added in the header file (same for return values).
For both of these cases doxygen tags usually come in handy.
If you are handing over an object allocated with new via a pointer (which means that the ownership/responsibility to delete it is handed over), this must be clearly stated in the comment section of that respective function. Note that you are still supposed to avoid this kind of construct whenever possible.
Note: Ignore the paragraph above when working on Qt code. That is a mess anyway.
And if a code block within a function does something that can not be understood from within the context of the function, you should consider adding a short comment too.
4. MWWorld::Ptr::getTypeName
Avoid calling this function if you can. It exists primarily for the use in the MWWorld::Class implementation. Its use in other parts of the code should be minimised. Particular bad are if else cascades that check for different types of objects, but even checks against a single type aren't great. Try to avoid them. We have the MWWorld::Class hierarchy to deal with this kind situation in a (somewhat) clean polymorphic way.
Consider what happens should we have to add a new referenceable record type (after OpenMW 1.0). This would obviously require a new concrete subclass of MWWorld::Class. And ideally most of the code specific to this new record type should go into this class. But with getTypeName spread out all over the code base, we have to hunt down all these locations. Now grep can help with that, but it is still an invitation for a massive bug invasion.
And that is all for the time being. I hope I didn't bore you too badly. Back to coding.
There is no need for full scale code review and improvement (isn't that bad yet), but please keep the following points in mind when working on OpenMW:
1. Code Duplication
Avoid it. If you have the same code twice in multiple locations, that is a clear maintenance disadvantage. More work for people who need to change the code, more potential for creating bugs (by having the two copies getting out of sync) and generally needlessly large code. We are not some derpy proprietary software development company where the quality of your work is measured by the number of lines of code you produce. So please think twice before you use copy and paste.
2. Don't litter!
Seriously, guys! Clean up behind yourself. Least significant problem of the four listed here, but it aggravates me the most.
In the early days we had a developer who produced functional code in a decent quantity, but the files he touched always looked as if a bomb had exploded. Our situation is not that bad yet, but we are getting there (slowly).
Please remove unused code. Functions and local/member variables that are not in use anymore, but are not outcommented, are a real problem. They slow down people trying to read through the respective part of the code. Also, they sometimes produce warning messages, that I have to fix later (since we are aiming for warning-less code).
Even outcommented code is a problem. Because it blows up the source files unnecessarily and still somewhat slows down people trying to understand the code.
The only point where I have no problem with unused code is if that code is basically fine, but can't be used currently, because somewhere else something is broken. In this case however you should add a comment to the unused code that describes the problem.
For everything else we have git. If you are dealing with unused code that you might want to make use of again some day and you are worried, that it might get lost in the vast caverns of the repository, then just add a tag to it.
git also offers tools to detect messy code. git status before a commit is pretty much a must. But you might even consider doing a git diff after a particular long and complex coding session that is more likely to have unused leftovers.
3. Comments
If you have a look at our Ohloh Page, you will notice, that we don't have a lot of comments in the code. I am actually fine with that (they call me a bloody minimalist for a reason) and generally I don't give a rat's arse about this kind of metric.
However it does seem that we are missing important comments in a few specific areas.
If a function does something that is not obvious from its name and its general location in the system, then a short comment should be added in the header file.
If a function parameter has particular constraints or other oddities then a short comment should be added in the header file (same for return values).
For both of these cases doxygen tags usually come in handy.
If you are handing over an object allocated with new via a pointer (which means that the ownership/responsibility to delete it is handed over), this must be clearly stated in the comment section of that respective function. Note that you are still supposed to avoid this kind of construct whenever possible.
Note: Ignore the paragraph above when working on Qt code. That is a mess anyway.
And if a code block within a function does something that can not be understood from within the context of the function, you should consider adding a short comment too.
4. MWWorld::Ptr::getTypeName
Avoid calling this function if you can. It exists primarily for the use in the MWWorld::Class implementation. Its use in other parts of the code should be minimised. Particular bad are if else cascades that check for different types of objects, but even checks against a single type aren't great. Try to avoid them. We have the MWWorld::Class hierarchy to deal with this kind situation in a (somewhat) clean polymorphic way.
Consider what happens should we have to add a new referenceable record type (after OpenMW 1.0). This would obviously require a new concrete subclass of MWWorld::Class. And ideally most of the code specific to this new record type should go into this class. But with getTypeName spread out all over the code base, we have to hunt down all these locations. Now grep can help with that, but it is still an invitation for a massive bug invasion.
And that is all for the time being. I hope I didn't bore you too badly. Back to coding.