Managing MegaGlest's code

[Omega]

Now that I've actually tried to implement things, I've found that MegaGlest's code base is a mess.

In particular, there doesn't seem to be much for code conventions.

• There's tons of commented out code
• Formatting is very inconsistent. In the same method, I've seen `foo=1`, `foo= 1`, and `foo = 1`.
• Documentation is virtually non-existent. This creates a huge learning curve to try and figure out what the software is doing. Not to mention you'll never understand the code you wrote six months later.
• Use of outdated C++ conventions. There's a complete overuse of pointers, for example. RAII, a common programming idiom in C++ for writing safe code, is rarely used. And new code pretty much has to be written pointer heavy, or it ends up being inconsistent with the rest of the code.
• There's a number of code clones. Some which can be eliminated by extracting common code into a method.
• Code is thrown into a handful of gigantic files instead of being appropriately split up. Smaller files are typically more manageable and less cumbersome to work with.
• Lines are pretty much never wrapped. I haven't even seen the majority of files, but I've already seen plenty of lines excess of 500 characters!
• Unit tests pretty much don't exist. MegaGlest would be difficult to test because (1) it was never written with testing in mind and (2) being a game, there's a number of things that only show up graphically (difficult to write automated tests for)

My point being that there's a lot we could do to make the code more manageable. Have some of these points already been considered?

Some of these can be addressed quickly. A formatter ("pretty printer") can make short work of inconsistencies in formatting. A great deal of commented out code can be removed quickly with appropriate regex (commented out code adds nothing; there's no way to know if the code works, if it is still applicable, if it has bugs, etc). Of course, the problem with these large, automated changes is that they pretty much cut off easy access to old revisions and make merging hell (tons of minor inconsistencies). It can be manageable if done when there's no major feature branches on a stable release, though.

Unfortunately, the more useful fixes, particularly documentation and use of modern C++ idioms, would be time consuming and require manual work. It's very possible, though (in fact, my current job is mostly refactoring, documenting, and creating tests for a Java project that did none of these).

[titi]

This is all well known  but you already mentioned several reasons why it is as it is. The main problem is, that the code already looked a lot like this when we started and it turned out that its very easy to break things.

But I know these problems from work where I often have to deal with old code. You cannot cleanup everything  and so its often best to adapt the same coding style you find in such a project. But you are right, we should really think about some of the points.

From your list:

Code formatting:
This is something I really want. I think softcoder and me use eclipse ( maybe you should try too ) to build. With this formatting can be done very comfortable! But this breaks the whole history so the point where we can do it must be choosen very carefully.  ( ideally no branches and nothing, so best moment is after a release maybe .)

Commented out code:
This is nothing that concerns me too much as its not in too many places ( maybe those regions you visited  are very worse ). It sometimes holds unfinished features or whatever. In general I prefer a manual way to clean this up if its really useless code.

Documentation ... 
Well, I try to comment at least a bit, but original code nearly had nothing commented. Documentation in general is something whats not too fun to do, especially giving a system overview of something you did not do yourself and where you don't know the details.  And I  still have problems to understand the code in many places ( especially OpenGL parts ). But if there is "someone" out there who wants to start with it ....

Use of outdated C++ conventions:
Well its old code, no wonder its in there. The problem is, if you start with something else you have a mixture of different coding styles making it even harder to understand the code in the end. GAE did this and it was really hard for me to understand the code mixture there.

Code clones:
Indeed not good and we should try to eliminate them.

Line wrapping:
I think its pretty much the same like code formatting.

Unit tests:
Well softcoder started a bit with unit tests. I personally think tests are really a lot of work and are typically not fun to do :-/ , so especially writing tests for already existing code which was not made by you  is not really something that will likely happen.
But there are some parts in the code where it would make sense. ( pathfinder fro example )

[Omega]

This is something I really want. I think softcoder and me use eclipse ( maybe you should try too ) to build. With this formatting can be done very comfortable! But this breaks the whole history so the point where we can do it must be choosen very carefully.

I actually use Eclipse as my dominant IDE for work and school. A current project of mine uses an on-save auto formatter. Although I think the Windows project needs some heavy tweaking to get it to work with Eclipse.

Documentation ... 
Well, I try to comment at least a bit, but original code nearly had nothing commented. Documentation in general is something whats not too fun to do, especially giving a system overview of something you did not do yourself and where you don't know the details.  And I  still have problems to understand the code in many places ( especially OpenGL parts ). But if there is "someone" out there who wants to start with it ....

I can try and add documentation to portions of the code. I don't really have any experience with graphics programming, though, so probably won't be able to touch that stuff (perhaps that'll change in the future -- taking a course on computer graphics next term).

Well its old code, no wonder its in there. The problem is, if you start with something else you have a mixture of different coding styles making it even harder to understand the code in the end. GAE did this and it was really hard for me to understand the code mixture there.

Good point.

Unit tests:
Well softcoder started a bit with unit tests. I personally think tests are really a lot of work and are typically not fun to do :-/ , so especially writing tests for already existing code which was not made by you  is not really something that will likely happen.

Yeah. I would think that testing would be easier if we could refactor methods to be cohesive (there's a lot of methods that do so much at once, and that's a pain in the ass to test) and make appropriate use of easily testable code (dependency injection is very helpful here), then creating unit tests with the help of a mocking library could be doable. But you're right that it's a lot of work and not very fun (well, except when the tests let you know that you broke something).

One thing in particular that we could do right now without too much work would be to split files into smaller ones. A bunch of files have several classes in them. Those classes could be moved to their own files.

[Omega]

Ok, given that I'm the one making the complaints, it seems that I should also be "putting my money where my mouth is".

So I've started an attempt to begin documenting the code. I've summed it up [in this pull request](https://github.com/MegaGlest/megaglest-source/pull/20). I hope to add more documentation over time (although it's going to be a long term thing). Due to this, I'd like to suggest that documentation changes are merged periodically. I'll make sure that the documentation branch contains no code changes, so it should be safe to merge at any time. Although, if time allows, it'd be a good idea to skim over the documentation before merging it, to catch any potential errors (I didn't write the original code, after all, so can only make educated guesses towards its intent).

I'd also like to propose that all future code is documented along the same lines of verbosity (and if you check the change request, you'll see that isn't much). It may also be a good idea to document complicated parts of the code as you make changes to them, as you'd be in a good position to understand that part of the code.

EDIT: I've further added documentation to another file. I've found, however, that I'm not sure how to handle potential easy refactorings that come up while documenting. For example, Upgrade has a factionIndex field that I believe is unnecessary. As far as I can tell, it's only used for checking that the Unit we're applying UpgradeManager::computeTotalUpgrade to is in this faction. However, I don't see an circumstances when it wouldn't be (since the UpgradeManager already an aggregate of a Faction and Unit directly gets the UpgradeManager from this Faction. Thus, it must have the same Faction as the Upgrades in the UpgradeManager).

For another example, the enum UpgradeState has an unnecessary value for the length of the enum (I never understood why arrays and enums in C++ didn't store their length, like every other language does), but the value is unused and there's no reason to iterate over the states.

For now, I'm just adding TODO notes in the comments (I don't want to mix potentially unsafe refactoring changes with the safe documentation changes). They can probably be done after the documentation changes (this particular one is very easy -- remove the field, change the constructor to not use that field, and change the implementation of UpgradeManager::computeTotalUpgrade to not bother checking that).

[Baŝto]

I agree, we need to find some standards and apply them bit by bit.

Documentation is definitely necessary and I also documented most headers of my map editor. But I will improve that within the next days, I need to remove some too obvious comments and maybe add some others or reference to others.
I will start a documentation branch and document while I stumble upon header files.
I hope I don’t break git when I pull omega’s commits for this.

Some methods are really hard to guess, like

Code: [Select]

void load(particleFileNode, dir, path, newTexture,loadedFileList,string parentLoader,techtreePath) of unit_particle_type.h, where path needs to start with dir.

For refactoring methods I would use tools like ack to find out where this method gets called and fix them.

I also used many pointers in my editor’s classes, but mainly because I did not understand how to get a new instance of a class without a pointer. But will do better in future code.

For formatting I would suggest:

4 spaces indentation
80 characters
trying to split methods if they exceed 80 lines

and methods like

Code: [Select]

int MyClass::myFunction(Type firstParameter, Type second){
    aMethodWithWayTooManyParameters("something", 1337, "/a/path/to/somewhere",
                                    new UglyClass(), new anotherUglyClass() );
    return ( (1 > 2) ? (4 + 5) : (1337 / 42) ) ;
}

[Omega]

I hope I don’t break git when I pull omega’s commits for this.

My documentation changes are in the master, now. I recommend that you follow the same style of documentation (Javadoc style with Doxygen). It's fairly readable in the source code and produces useful documentation when combined with the Doxygen tool.

For refactoring methods I would use tools like ack to find out where this method gets called and fix them.

As an aside, an IDE's "call hierarchy" feature will usually do a better job at this (as it actually parsed the files rather than blindly searching). With Eclipse, the call hierarchy is very fast due to how Eclipse indexes the files. Most IDEs should have a feature like this (although some, such as MSVS 2010, are incredibly slow and possibly inaccurate).

I also used many pointers in my editor’s classes, but mainly because I did not understand how to get a new instance of a class without a pointer. But will do better in future code.

If you're interested, here's a relevant SO question and here's a portion of Stroustrup's C++ book that deal with RAII and its benefits.

For formatting I would suggest:

4 spaces indentation
80 characters
trying to split methods if they exceed 80 lines

I would disagree slightly. I'd like to suggest:

• Tabs for indentation (allow adjustable indentation levels; removes argument about size of indentation). Spaces can be used for alignment after indentation is done. Tabs should *never* be used for alignment, as they have variable width and in some editors, tabs indent to the next tab stop level while in others, they indent a set width. Although frankly, anything's better than mixing tabs and spaces on the same line...
• When wrapping lines, indent the wrapped line with 2 tabs.
• 100 character widths. 80 characters seems too small, and the majority of screens I've seen in recent years can easily fit 100 characters, even with IDE windows. More than that is probably a bad idea, though, as we'd want to avoid horizontal scrolling.
• Stroustrup style indentation and braces. We're already mostly following this, but are inconsistent in a few places and the spacing is extremely inconsistent.
• Space between all binary operators. That is, foo = x + 1 instead of foo=x +1 (etc).
• In conditionals, don't compare a boolean to true or false. ie, use if (foo) instead of if (foo == true)

I'd also like to suggest the following non-formatting changes:

• Stop using C style strings. That means no printf. Mostly to avoid mixing C and C++ style strings (and IMO, C style strings need to burn in hell)
• Those places with 10 indentation levels? They need to be split up or other wise refactored. You won't even be able to wrap these without them going off the screen.
• Remove all the commented out code. It's already in the repo if anyone ever needs it. I suspect a great deal of it will never even be looked at, though. As it is, it's just cluttering things up. The author didn't even mention why it's commented out. The code may also be buggy or outdated.
• Prefer to qualify std members with their full namespace. Eg, std::vector instead of just vector.
  

Here's an example that just shows the formatting. ---> represents a tab while · represents a space.

Code: [Select]

/**
·*·Does·some·cool·thing.·More·details.·Yet·more·details.
·*·@param·foo·Some·foo.·Details...
·*·@param·bar·Some·bar.·Details...
·*·@return·Some·non-sense·because·I·wanted·to·demonstrate·all·the·formatting·points.·Oh,·and·this
·*·line·is·approx·100·characters·long.
·*/
int·someFunction(Foo·foo,·Bar·bar)·{
--->if·(foo·==·bar)·{
--->--->std::cout·<<·foo.getName()·<<·"·equals·"·<<·bar.getName();
--->}
--->else·if·(foo.thisLineIsVeryLong()·<·bar.toShowLineWrapping()·&&·foo.hasTehCodez·&&
--->--->--->!bar.shouldDefinitelyBeFalse())·{
--->--->//·...
--->}

--->return·foo.shimShimmyShoo(bar);
}

//·Stuff·that's·aligned·--·I·actually·dislike·aligning,·though
int·getMeaningOfLife()···················{·return·meaningOfLife;·}
void·setMeaningOfLife(int·meaningOfLife)·{·this->meaningOfLife·=·meaningOfLife;·}