Classes refactoring

International board Development
#1

Hi guys,

as you may have noticed there is a new branch called classesRefactoring1 in the SVN. It’s the first part of a major code refactoring and compilation unit re-organization. It will take some time to finish those steps and I hope it goes off without a hitch.

This branch includes following changes compared to the trunk revision 2479, which is at the time of writing the most recent trunk revision:

[ul]
]Introduction of pre compiled header files. STL, iostream, boost libraries, vstd library, logger functionality and some macro resides in those files.
/:m]
]Redundant include directives have been removed.
/:m]
]Include guards have been replaced consistently with #pragma once.
/:m]
]THex is now named HexField(should be named CHexField, sth. for next revision) because of a naming clash with the Hex Field class of the battle interface. You can find HexField in /lib/HexField.cpp/.h. Thats means it has now it’s own compilation unit.
/:m]
]Constants and enums have been moved to GameConstants.h. They should not be moved into PCH, because they are likely to change. Not every compilation unit should get compiled when changing this file.
/:m]
]VCMI_DLL macro has been removed. That constant is now set directly at project pre compiler definitions.
/:m]
]Macro DLL_EXPORT has been renamed to DLL_LINKAGE to avoid confusion.
/:m]
]RegisterTypes.cpp is now located in .h file. Things changed a lot and some macros related to it get unnecessary.
/:m]
]Compiler warning set to level 3
/:m]
]All project parts(client,lib,…) on Linux and Windows have now the same compilation units. Server and Client on Linux had CConsoleHandler and ThreadHelper, which was different on Windows with VS project settings. Those files are now part of the lib project consistently included with some other header files(int3.h,StartInfo.h,…)
/:m]
] I’ve removed nodrze.h and the hch folder as they were unnused. Is this okay?/:m][/ul]

Above points help mostly to reduce compile times, but this wasn’t my actual intention why I’m started the whole stuff. I mainly wanted to have two things. A better separation of classes and a consistent naming scheme of classes,enums,structs,… This is needed to keep the code consistent, to introduce some guidelines we all follow and the most important point to have things quite clearly separated. This gives us a good base for further code maintenance / refactoring and mod support.

The first part split up the classes which reside formerly in the CBattleInterface.cpp/.h unit and can now be located under client/BattleInterface. I tried to change at least as possible, so normally you can work further mostly the same way as before.

Comprehensive list of old names and new names of those classes:

CBattleAttack = CAttackAnimation
CBattleAnimation = CBattleAnimation
CBattleConsole = CBattleConsole
CBattleHero = CBattleHero
CBattleInterface = CBattleInterface
CBattleOptionsWindow = CBattleOptionsWindow
CBattleResultWindow = CBattleResultWindow
CBattleStackAnimation = CBattleStackAnimation
CDefenceAnim = CDefenceAnimation
CDummyAnim = CDummyAnimation
CBattleHex = CHexFieldControl
CMeleeAttack = CMeleeAttackAnimation
CBattleStackMoved = CMovementAnimation
CBattleMoveStart = CMovementStartAnimation
CBattleMoveEnd = CMovementEndAnimation
CReverseAnim = CReverseAnimation
CShootingAnim = CShootingAnimation
CSpellEffectAnim = CSpellEffectAnimation
SStackAttackedInfo = SStackAttackedInfo
CStackQueue = CStackQueue
SStackAttackedInfo = SStackAttackedInfo

Next steps
I don’t know about the exact course of events if we do the next steps first only for the branch and then merge with trunk or incrementelly merge branch and trunk several times.
@Tow: Could you say which way is the best to get this done right?

I don’t want to give a time schedule, as it’s not easy to say. Next steps are to split up further units like GUIClasses, AdvMapInt,… and to re-arrange them properly.

Compile times
Lib
Branch: 2:03min, Trunk: 2:56min(vc9);
Branch: 2:10min (vc10);
Branch: 2:47min, Trunk: 3:25min(gcc);

Client
Branch: 2:15min, Trunk: 3:05min(vc9);
Branch: 1:56min (vc10);
Branch: 6:42min, Trunk: 7:22min(gcc);

Server
Branch: 0:53min, Trunk: 0:38min(vc9);
Branch: 0:44min (vc10);
Branch: 0:51min, Trunk: 1:09min(gcc);

Genius
Branch: 0:25min, Trunk: 0:39min(vc9);
Branch: 0:22min (vc10);
Branch: 0:29min, Trunk: 0:32min(gcc);

Stupid
Branch: 0:11min, Trunk: 0:09min(vc9);
Branch: 0:14min (vc10);
Branch: 0:15min, Trunk: 0:09min(gcc);

ERM
Branch: 5:39min, Trunk: 3:45min(vc9);
Branch: 5:05min (vc10);

Compile times describe the time for a full rebuild. Normally you wouldn’t do that with PCH files. They should never change or only rarely. Compilation of PCH file took 10seconds on gcc and 5 seconds with vc++, so you can substract that time from all measured branch values. GCC profits most of PCH files. Client on gcc took so much time because of the slow compilation of the musichandler.cpp. ERM took much longer with PCH, don’t know why, perhaps it doesn’t make so much use of the boost library which are defined in the header file for that project. This could be adjusted. But in general compile times are definely shorter.

Coding guidelines
With that branch we also introduce a set of coding guidelines. You can see them in the wiki.
wiki.vcmi.eu/index.php?title=Coding_guidelines

They aren’t carved in stone yet, so you should wait some days till they are final.

Have a nice day,
beegee

#2

Interesting enterprise. However, I have some objections:

Terrible name. It’s now not clear whether it’s hex, field (?) or control. Battle hex is much more clear.

Splitting animations to different files make no sense, as they are similiar, short pieces of code and inherit from same source. One file for BattleAnimation would be well enough.

As to game constants:

Some of them should be finaly variable, handled in some mod engine. Fox example:

const int CREEP_SIZE = 4000; // neutral stacks won't grow beyond this number
const int WEEKLY_GROWTH = 10; //percent
const int SPELLBOOK_GOLD_COST = 500;
const bool DWELLINGS_ACCUMULATE_CREATURES = false;

Others define something I will call a “module” from now on.

const bool STACK_EXP = true;
cconst bool STACK_ARTIFACT = true;

Modules are parts of engine that handle some complex functionality and will be base for other mods and modifications - objects, artifacts, or scripts. In the end modules need to be handled by mod engine as well, but their influence on game is significant and they deserve a spotlight.
Other possible modules I can think of are Commanders, Mithril. probably also other popular modifications, like Henchmen.

#3

It’s a UI control element which represents a hex field.(hex field goes together)
Damn, I’ve discovered that the english equivalent to Hexfeld in german is hex cell or simply hex. Ok, perhaps it’s better to change the name of the control hex cell and the logical hex cell.

Proposals:
Grahpical hex cell: CBattleHex => CHexControl or simply the same?
Logical hex cell: SHexField => SHexCell or simply SHex?

Mhhm. Yes, they are similarity concerning that they’re animations. But they handle different types of animations and they aren’t comparably. I think it’s better to separate that. I think it doesn’t matter how much lines the code has. For example I try to move the IUpdatable, IShowable,… interfaces in a single file in each case. As they’re supposed to be used and to be implemented by “main” classes, they need to have high visibility.

#4

How about 'BattleHex" and “ClickableHex”? These are self-explanatory I believe.

Yes they are, they all handle cretaure animations in battle and, well, inherit from CBattleStackAnimation.
Better keep them in one file, as managing hundreds of files at once will eventually become impossible.

#5

These are good names, thanks.

Okay, I’ll pack animation classes in one file as they fit together well and you’re right they get unmanagable. Is the compilation unit name “CBattleAnimations.cpp/.h” ok? Is it better with “CBattleStackAnimations.cpp/h”.

We shouldn’t make the mistake to pack too much classes in one file, but we should also look not to create too many files. This isn’t easy task, escpecially when it comes to GUIClasses.cpp:)

#6

Some of them like DATA_DIR should be possible to define from command line on startup and/or from some config.

CCreatureAnimation.cpp/h should go in that directory too - it is only used for drawing during battles.

There is also client/vcmi_client directory with ancient netbeans project files (same for lib and server) and really outdated codeblocks files (vcmi.workspace in trunk and *.cbp in client/lib/server/genius)

Instantiation of that 100-line defines with all music\sound files in combo with boost::assign_list_of is insanely long :frowning:
Haven’t found any way to fix that.

One note about coding guidelines:

What about moving comments in separate line like you’re proposing for methods? I don’t think that line count of headers is significant with modern IDE’s.

#7

StdInt files look horribly copy-pasted. I think we should have a master StdInc file where most of the stuff would be placed. ERM interpreter build times woud probably be better if its StdInc would have boost.spirit in it (it uses this library quite much).

I have no preference for this, we could do it both way depending on situation/coder.

#8

Why not restore global.h and simply include it into all these StdInc.h files?

I do like looking at the class definition and being able to see its members. I’d suggest to keep the comments always in-line, unless it needs to be spanned across several lines. But short, several words descriptions, are fine in line.

C++11 initializer lists are the nice way… Unfortunately not supported by Visual Studio (neither 2010 nor the upcoming release). Shame on Microsoft! :frowning:

What about adding directory paths to GVCMIDirs? Initialized with default macro and overwritten by command-line args.

I’m afraid that multiple files will be difficult to manage. It’s much harder to refactor a class having its own file than a one stored in a container file. Renaming files is troublesome (needs an update of VS project, makefile and proper command to SVN).

Look promising, good thing to have done. :slight_smile:

Yes.
[Though I have so many memories with nodrze… :stuck_out_tongue: ]

I’d rather improve the branch and then do a single, big merge. It may cause some trouble (big changes to trunk), it’s better to have it done once.

And what about improving battles? AFAIR it was the purpose of the refactoring.

@coding guidelines

  1. It should say somewhere at the beginning — code duplication is the worst sin. Avoid if possible.
  2. if the comment duplicates the name of commented member, it’s better if it wouldn’t exist at all. It just increases maintenance cost. Eg (bad):
size_t getHeroesCount(); //gets count of heroes (surprise?)
  1. I’m not sure about recommending using size_t for iteration index. It’s very easy to make a mistake then when reversing the iteration order. It caused troubles for me several times.
  2. We don’t put space after if. The keywords are highlight and I don’t think they need further separation.
#9

Sorry for the delay. I’ll answer tomorrow. It’s much to read and to think about. I have to find arguments pro a good separated class design. :slight_smile: Would be great if we could discuss things in RL.

@Tow dragon

What do you exactly meant with horribly copy-pasted? What things could be done to improve it? I’ve thought that now it’s more cleaned up than the former global.h header file.

#10

Look at this fiile. Then compare it with this one . There are several more files like those two. If I were to write the most important lesson I’ve learned coding it’s the DRY principle. The second would probably be “be comprihensible” but it’s not important now. Count how many times did you write the same thing. There are seven identical files. They are not even supposed to diverge, I think. The only thing worse than creating seven identaical files I can imagine is probably creating even more identical files – no programming construct can be worse. I would even accept a module in Malbolge if it worked but such multiplication of code is obviously asking for trouble with no gain.

#11

@Tow dragon:
Okay, now I know what do mean. Yes that common parts of those StdInc.h files have to be gathered somewehere let’s say Global.h. My first approach was to set up one common pre compiled header file for every project, but due to some annoying problems with Visual Studio this wasn’t a good solution. I didn’t get it working with VCMI. Lib compiled without any problems, but i’ve got some very mysterious SDL, boost linking errors and errors about the EBuildingState::BUILDING_ERROR enum when compiling vcmi_client. social.msdn.microsoft.com/Forums/en-US/vclanguage/thread/665d4183-f85c-481d-bada-03283b310099/
Don’t ask me why I did this, perhaps because I’ve lost too much time configuring PCH properly, I’ve placed the very same file in every VCMI project. But I also think that it wouldn’t be that much bad to have it like that way, because PCH files shouldn’t changed ever or only due to some very special cases. But I also think that it’s better to have one Global.h file which collects common includes.

Class - file - handling

If you rename a file you should also rename the class name which resides in that file. I think this is much more troublesome escpecially in C++ than to update the VS project, makefile and SVN which is done in one minute.

I think it’s better to have a one to one relationship between the class and the c++ unit.(cpp/h).

  1. If you have to search a specific class you’ll find it faster and even without any IDE.
  2. Compilation time can be shorter if you just change some code in a CPP file.
  3. I think it’s definitely not good to have a CPP file with 3000+ lines of code if it could be done better. Sometimes you don’t want to jump exactly to a method you know but you want to simply explore the code and scroll down with the text editor and look what’s in it. Perhaps you’ll see more likely bad smelling code.
  4. It doesn’t matter which class definition comes first and you don’t have to worry about forward definitions if every header file has specified its used resources(enums, classes,…)
  5. SVN history texts are better separated. If you change one thing on one class in a big unit like GuiClasses.cpp, another dev is changing another thing in the same file, then the former change text gets oversight fast.

I also think that one file per class could also be bad when you got a huge project with a total LOC of 150.000+. Then it may be better but not for every case to have 5 to maximum 10 class definitions in one file, where one class would have not more than 500 LOC. Many files can get unhandy or slower the compile time for a complete rebuild. I’ll keep you informed how the class file separation process goes on. I think that it is good for the moment to have those animation classes each in one file.

It was one part of the refactoring, yes. But it is bad to have the BattleInterface split up and the rest not. It’s much work left to do, so I’ll commit GUIBase changes and the suggestions above in that thread, stop doing the separation process and then I’ll turn my attention to the battle interface. I think that’s the best way. So I’m not overwhelmed with massive refactoring but can also fix some bugs or improve some other things.

#12

Rev 2494 improved several points which were named above. There exists now a new folder called UIFramework and some files in it which has been built up by GuiBase.cpp/.h. All classes have the same name except for those which doesn’t follow our naming scheme.

What’s up with the hch folder and the DPIaware.manifest file? Can they be removed?

#13

I think all hch files were moved to lib few months ago.

#14

There are valid arguments both for splitting and for not splitting. If there weren’t, we’d have a separate .cpp for each method or kept the whole project in a single file. Where the optimal compromise lies is the matter of personal preferences. If you’re willing to work with battles GUI then you can split it as much as you want (you’ll work on that code) but don’t enforce controversial changes unto the rest of code. (one file, one class rule is ATM controversial)

AFAIK we’re somewhere around 100k. :slight_smile:

Please don’t remove, it was needed: [forum.vcmi.eu/t/vcmi-0-75-released/248/10)

#15

Okay, let’s make a compromise. In my opinion it’s bad to have different ideas/realizations with regard to class/file handling spread among the project. What I really wanted was at least to have some consistency in that point. I’ve quickly developed an idea, which isn’t so strict in terms of one file per one class and scales perhaps better in the future. But I don’t know if it’s really good, so I’ve thought of posting these ideas first before starting programming.:slight_smile:

  1. Folder BattleInterface:
    CBattleAnimations.cpp/.h (holds all battle animation classes)
    CBattleInterface.cpp/h. (holds the class CBattleInterface+SStackAttackedInfo.h)
    CCreatureAnimation.cpp/.h
    CBattleInterfaceClasses.cpp/.h (holds all other classes, CBattleHero, CClickableHex, CBattleConsole,…)

  2. Folder UIFramework:
    CIntObject.cpp/.h (holds all interfaces, ckeyshortcut)
    CGuiHandler.cpp/.h (CGuiHandler plus FPSManager from sdl_framerate.h(should be renamed to CFramerateManager or so)
    SShapes.cpp/.h or SGeometries.cpp/.h (SRect, SPoint)
    CIntObjectClasses.cpp/.h (holds all Button classes, CSlider, CPicture,…)

Does this goes into the right direction?

I would say 5 to 10 classes should be the maximum count of classes in one file. No unit should have less than 500 lines of code. I hope it’ll be okay for everyone.

@Tow: Can I rename those polish named colors? zwykly… There are 4 color constants in CMessage.cpp and 2 in CGuiHandler.cpp. Those former four pollute global declaration scope. Perhaps it would be better to define these 6 color constants in a namespace called Colors placed in the SDL_Extensions.cpp/h file as those colors are of type SDL_Color.

I hope we’ll get a good compromise soon because I think this is important and I really want to do more interesting things where the gamer of VCMI benefits too.:slight_smile:

#16

From the consistency point of view it’s better to have whole project written using the same style rules. However when it’s difficult to reach an obvious consensus, I don’t see anything wrong in allowing a reasonable level of diversity. Files varying in count of contained classes are not a problem.
If you prefer working with a single class files, it’s all right with me, if you split battle interface classes. I have no plans of spending any significant time on them in predictable future anyway. It’ll be your code to maintain, so primary you should feel comfortable with it. However it’s a different thing when your are refactoring a whole project codebase. There every change will affect everyone.

It somehow looks strange with that S prefix. So far almost all structs were without any prefix (just written with a big letter). I’m not convinced, if it should be introduced — it would cause big fuss if introduced everywhere (we have quite a many structs), partial introduction seems only increasing inconsistency. I think rather about removing those few structs that have S prefix. Rect and Point looks to me much clearer then SRect… though maybe it’s only a matter of being accustomed to.
This thread is becoming too much of a clash of two developers’ tastes and customs. What do other developers think? :wink:

Looks acceptable. :slight_smile:

Certainly. I somehow even thought it was already done but obviously it is not. Old, very old code.

#17

I think they would already write here if they cared or thought they should influence the coding standards discussed here.

#18

well begee looks like he started programming on 8-bit machines with basic!
no need to declare variables, and we know if something ends with $ is a string if with a % is a number (it was integer? and float has other type-sign?)

yeah that was easy but there wasn’t any object programming, so now with thousands types (including object ones) and bilions of individual variables it feel it would fail, so most easy is just to provide descriptive names

#19

…
…
What???

Shouldn’t this be the other way around? Beegee is apparently more fond of object programming good practice rules than I am.

#20

well some habits are really hard to avoid, and while begee may be really good at objective programming, he keeps the habit for code-marking variables (and types since OOP) with more machinal than logical way (and strictly avoid duping of symbol functionality “it’s not class but struct!” - rather than stating C symbol is just for some object-type)

well i don’t say he does something really wrong or fail competition with OOP, but only state one habit which in my see origins from early (non-OOP) user friendly languages which once ruled 8bits “computers”