Code Quality Issues

Everything about development and the OpenMW source code.
User avatar
Zini
Posts: 5538
Joined: 06 Aug 2011, 15:16

Code Quality Issues

Post by Zini »

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.
User avatar
Jyby
Posts: 408
Joined: 10 Dec 2013, 04:16

Re: Code Quality Issues

Post by Jyby »

I've looked at a bit of the code and it definitely needs a bit more focus on maintainability/documentation.

But I did find this:

zinnschlag 3 years ago implemented take action
cell.hpp

Code: Select all

/// Make the cell visible. Load the cell if necessary.
virtual void show() = 0;

/// Remove the cell from rendering, but don't remove it from
/// memory.
virtual void hide() = 0;

/// Destroy all rendering objects connected with this cell.
virtual void destroy() = 0;

/// Make the reference with the given handle visible.
virtual void enable (const std::string& handle) = 0;

/// Make the reference with the given handle invisible.
virtual void disable (const std::string& handle) = 0;

/// Remove the reference with the given handle permanently from the scene.
virtual void deleteObject (const std::string& handle) = 0;
This is how you should document your code! C++ is unlike Object-C, we don't intuitively write to make it readable, so you need to understand that your code needs to be describe in plain english as well, especially for an open source project.

I know its very difficult to enforce quality with the work force we have, but I would be happy to start/organize a QA group to review code, just point me in the right direction.

Cheers,
Jyby
wheybags
Posts: 207
Joined: 21 Dec 2012, 19:41

Re: Code Quality Issues

Post by wheybags »

According to that ohloh page, we currently have 99 people in our git history :D
User avatar
taknamay
Posts: 68
Joined: 01 May 2013, 13:22

Re: Code Quality Issues

Post by taknamay »

wheybags wrote:According to that ohloh page, we currently have 99 people in our git history :D
But a glitch aint' one.
corristo
Posts: 495
Joined: 12 Aug 2011, 08:29

Re: Code Quality Issues

Post by corristo »

How about adding some automatic code linting to Travis config? Things like that should be automated as much as possible.
I think absence of comments could be checked too.

@Jyby
Objective C's "self-documenting" is overrated
maqifrnswa
Posts: 180
Joined: 14 Jan 2013, 03:57

Re: Code Quality Issues

Post by maqifrnswa »

taknamay wrote:
wheybags wrote:According to that ohloh page, we currently have 99 people in our git history :D
But a glitch aint' one.
wow... bravo
yes, it's been 4 months - but this quality can't be missed!
izackp
Posts: 4
Joined: 19 Jun 2014, 14:09

Re: Code Quality Issues

Post by izackp »

Here is just my two cents
Zini wrote:where we either have to stop it or give up on stopping it.
Its always a good idea to leave code better than what you left it. If you give up all together that means that the difficulty in maintaing the project will exponentially increase with time and up to a point where everyone will be afraid of making small changes in order not to break anything.
Jyby wrote:

Code: Select all

/// Make the cell visible. Load the cell if necessary.
virtual void show() = 0;

/// Remove the cell from rendering, but don't remove it from
/// memory.
virtual void hide() = 0;

/// Destroy all rendering objects connected with this cell.
virtual void destroy() = 0;

/// Make the reference with the given handle visible.
virtual void enable (const std::string& handle) = 0;

/// Make the reference with the given handle invisible.
virtual void disable (const std::string& handle) = 0;

/// Remove the reference with the given handle permanently from the scene.
virtual void deleteObject (const std::string& handle) = 0;
I wouldn't say this is the greatest example of commented code. Considering that this seems to be an abstract class the comments, if any, shouldn't say 'how' something is implemented. "Remove the cell from rendering, but don't remove it from memory." says how. When in reality someone may inherit/override it and not actually remove it from memory. Now there is a false assumption to trip up anyone else reading the code. Also the point of an abstract class is to not care about 'how' and care more about 'what'.

I agree with everything Zini has stated about comments. I would just like to add that, in my opinion, you should avoid comments as much as possible because people will change the code and not update the comments or they will write what their interpretation of the code is even though it maybe inaccurate. The best way is to make your code self explanatory where possible. However, except in the case where you have a 'public api' (like facebooks graph api) where everything should be commented as much as possible since its available to such a wide audience and the internal workings are unavailable for viewing. Or in the case of 'Side Effects' (which are evil).

Overall my point is, instead of thinking 'what should i write in a comment to help people understand this code' you should think 'How can I make this code more understandable.'

Anyways, I would definitely recommend to any software developer to read this book:
http://www.amazon.com/Clean-Code-Handbo ... 0132350882


Side note: I've tried created a project avoid runtime type checking all together and I've failed miserably when dealing with events :/

Also: As far as unused code goes, perhaps a linter can weed them out?
User avatar
MrAnonGuy
Posts: 42
Joined: 26 Aug 2016, 01:21

Re: Code Quality Issues

Post by MrAnonGuy »

Jyby wrote:code needs to be describe in plain english as well, especially for an open source project.
+1
izackp wrote:you should avoid comments as much as possible because people will change the code and not update the comments
-1
mattwla
Posts: 59
Joined: 17 Jul 2017, 14:45

Re: Code Quality Issues

Post by mattwla »

This was an interesting read, thanks for typing it up. I've got a question about point 4.

Code: Select all

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.
I'm curious how adding a new subclass of MWWorld::Class could mess with current uses of getTypeName. Is that because in order for getTypeName to function with the new subclasses, we would need to alter getTypeName, which could potentially create bugs in getTypeNames current uses? Also, why would we need to hunt down all current uses of getTypeName?

Thanks,
User avatar
raevol
Posts: 3093
Joined: 07 Aug 2011, 01:12
Location: Caldera

Re: Code Quality Issues

Post by raevol »

This sounds like a perfect case to use polymorphism... are we not doing that?
Post Reply