As I get into working on VCMI I recognized several things which bothered me and these are my thoughts about it. Don’t take them too seriously. I do not want to flame anyone here. =)
I’ve noticed that many source files of VCMI hold several classes. Sorry but this makes no sense to me, perhaps I haven’t heard anything of doing so. Common practice in Java, C# and I think even in C++ is to have one file for one class. In C++ this means naturally one .h and one cpp. file. Okay this is not always the definitve rule which have to be exactly followed, sometimes it makes sense to have two or three classes/structs in one file. But in general this isn’t so. The problem with the ‘put 30 to 40 classes in one cpp file’-method is that it gets fast disorganized and unstructured. For example the GUIClasses.h holds easy up to 20 classes.
VCMI consists collectively of 556 classes, structs and interfaces. I’ve measured that with Doxygen and Notepad++. The project has 80 header files. This means in average that there are nearly 7 classes in one header file. I think a good value would be maximum 1.5.
Further I’m not a fan of the current project folder organization. VCMI is software designed for the end user. Modding support can and should be provided by a scripting language like python. So why there is a GeniusAI.dll and a VCMI_lib.dll ? DLLs are primarly created for the need to provide functionality to other apps, there are not the solution to organize the overall project into smaller pieces. Projects divided into sub-projects can create dependencies and with them problems. Of course there is nothing wrong how it’s done at the moment but I think it could be done better. The VCMI_Server.exe and VCMI_Client.exe could then be merged. The server could be called by a specific parameter.
I think for a total of 556 classes there should be 20 folders created in which headers and sources are organized:
actions (select map, dismiss hero,…)
objects (cartographer, boat, dwelling,…)
interface/menu (level window, fort screen,…) > ingame menu, main menu, town menu, battle menu, adventure objects menu
ai > Battle AI, Adventure AI
general (global.h, main routine, setting up sdl,…)
The way files are organized should be the same how files are organized in the project solutions explorer in Visual Studio that everything is clear. Header and source files should be placed together in one folder in VS like it’s done in the GeniusAI project.
Such a refactoring would probably require many hours of work but I think it’s definitely worth the effort.
PS: What does actually the file naming convention with the preceding C means for CConsoleHandler,… and so on ?
Can you provide and maintain makefile with 500 files on different platforms? While half of game engine is being redesigned?
Some files have grown excessively, like ObjectHandler or GameHandler which cover lots of different game mechanics, but apart of that I can see only HeroHandler carrying too much unrelated content. Other files are consistent and handle specific funtionalities. In C++ we have headers and .cpp files for a reason.
C stands for class I suppose, in case it wasn’t clear enough. I stands for interface.
The main reason for having many classes in one compilation unit is lowering compile times. E.g. classes from mentioned GUIClasses share a lot of common code they depend on. Currently full rebuild takes about 6 - 7 minutes on my machine and I think it’s painfully slow.
Remember that some of them are just small classes for network communication or classes that are not used outside of a single compilation units (e.g. battle animation classes - btw. I just realized that their definitions should be in .cpp files instead of .h file). I agree that having two or three times as many compilation units would provide better structure but I suspect compilation times would grow by a similar factor.
GeniusAI.dll is separated because we wanted to support custom AIs, written and distributed independently from VCMI. I’m not sure about why VCMI_Lib.dll was separated, possibly because we didn’t want it’s files to be separately compiled for client and server. Finally client/server separation is an old design decision that I cannot explain right now but I think merging them would be more troublesome than leaving in current state.
Actually I doubt those refactorings would make much difference for people who have as good IDE as I or Tow do. When you can jump almost instantly between definitions of symbols, the way they are spread across files and folders is almost irrelevant.
C for classes, S for structures, T for typedefs, I for interfaces and CG for classes that underwent certain major refactoring in the past… but as you can see we are not strict about this convention.
No, we haven’t specified any guidelines for code styling. I we wanted to introduce it, we would have to make most of our code conform such rules which would be very time consuming and not particularly beneficial
Yes, it could be useful for new developers. If you make a patch, I can review and commit it. It would be better if this patch is prepared for bonusesFifthPart branch as it will soon be merged into trunk and contains almost all of patches committed to trunk.
@Tow dragon: Patch is finished, I’ve sent you a mail.
Update 14:00h: After further investigation I have to say that source files are really good written and commented perfectly. Thumbs up!!
I’ve commented class declarations with the 3 slash xml comment syntax (///) so that they can be recognized by Doxygen and also some text editors can display them in a different color. I suggest to set in VS2010 the color of XML Comments to blue-green.(0/128/128)
I have another proposal how to structure source code repositories, which shouldn’t be much work to realize it. Every trunk, branches, tags folder is composed in a way like that:
and primarly a src folder
That means leave everything which are resources (graphics, sounds, maps,…) in the root folder and move all sources files into a separate src folder. The src consists of following folders:
Global (for source files which are needed by all sub-projects, should be included into the include list at the project build path)
The source folders hold only .h/.hpp and .cpp files and nothing else. Visual studio project files, makefiles shouldn’t be distributed at all. Separately distributed project files e.g. for VS10/VS2008 are fine. Perhaps then it would be good to write an article how to setup and configure Visual Studio or another programming environment like Eclipse.
It’s great that the .hch folder is more or less banned with the newest trunk revision, so project organization gets now easier. In Eclipse you have just to add the Client folder for Client exe and needed global header files located in the root folder of VCMI to the list of linked resources. The project files can so be located wherever you want.
remove extremely outdated (2 years) project files - it will be more easily to recreate them from scratch or by importing from Visual Studio projects - most IDE’s support that.
CodeBlocks: vcmi.workspace and .cbp files in each folder
probably NetBeans files: folders vcmi_ in each folder
nodrze.h - not used anywhere, according to English comments this is implementation of RB-trees.
CConsoleHandler.h\cpp - move to /lib? Used by both client and server.
Looking on last commits by Warmonger - he added some files with in-game text. Maybe it will be better to add localizations support - folder with locale-specific config files or something like this.
Good idea. We should just warm everybody that project files are removed in revision xxxx to let everybody backup their project files.
I think we should put any new project files on svn… separate download will be better. Also keep in mind that such project files should be updated by a developer who actually uses that IDE.
Not used, and, what’s worse, not fully working. But Tow has a sentiment for that container (and on debug it was much faster than std::set).
If it’s used by both, it should be global, not in lib.
I don’t think we should split mechanics config files and language configs. Some files mix both information and texts. And, by the way, I’m not sure if providing good internationalization support would be easy.