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:)
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 
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.
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.
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!
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. 
Yes.
[Though I have so many memories with nodrze… 
]
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
size_t getHeroesCount(); //gets count of heroes (surprise?)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. 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.
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.
@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).
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.
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?
I think all hch files were moved to lib few months ago.
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.
Please don’t remove, it was needed: [forum.vcmi.eu/t/vcmi-0-75-released/248/10)
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.
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,…)
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.
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? 
Looks acceptable.
Certainly. I somehow even thought it was already done but obviously it is not. Old, very old code.
I think they would already write here if they cared or thought they should influence the coding standards discussed here.
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
…
…
What???
Shouldn’t this be the other way around? Beegee is apparently more fond of object programming good practice rules than I am.
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”
Could you give a few examples? It’d explain what you mean more easily.
Named types are much older than OOP (Algol 68 for example).
Do I understand correctly: you say that not naming each class with prefix ‘C’ is an 8-bit era habit?
what do you think about C++11?
constexpr, initializer lists, lambdas…
Not exactly…
I mean that differentating CSomothing and SSomething is a habit from very early user friendly languages, because then programmer was forced to code-type variables for gaining general benefit of language which were user friendly eg. no need to declare variable (first use of variable was an informal declaration, and no type keyword was needed because type was know by the name of var)
I don’t argue or call something bad, I just smell an anachronism which incorporates in one of devs’ mind. well maybe another flower smells
moreover C prefix is leftover from the 8bit era but its an useful idiom - CSomething is a type not variable, this idiom makes logic in naming types - but differentating between CSomething, SSomething, ESomething etc. is just anachronism where more stress is on mechanic of language than logic convention… but welll C suggest Class… I would better use TSomething but C idiom is much more frequently used…
Sorry if I’m harsh, or I’m stating inadequate conotations… I just well remember BASIC on my AMSTRAD CPC6128 - I was programming on it since i was 7 and during a few years (I can now write simple program without looking in instruction) - and stating those habit made me thought of those Good Old PLatform again.
well if I made offtopic earlier I say more - different times = different technics - and state an example MM9 was enforcing new technology too early - MM8’s 2.5D technics was perfect in these times - but 3do enforced full3d and made worst might&magic game - what they expected strong was really weak. well why not make experience enlongment? C or T idiom comes from early habits but makes same benefit. using C (class), S (struct), E (Enum), D (TypeDef) - each another is habit over benefit - so it makes anachronism.
I think it’s mostly a step in the right direction. Unfortunately C++ has already too many features and I’d like to see a new, redesigned “superset of C”, giving the feel that its author knows Lisp and type theory.
Hosting provided by DigitalOcean