Refactoring: move some of the constants into one header?

Questions, problems and discussion about compiling FreeOrion.

Moderator: Oberlus

Post Reply
Message
Author
User avatar
mem359
Dyson Forest
Posts: 214
Joined: Sun Jun 08, 2014 1:18 am

Refactoring: move some of the constants into one header?

#1 Post by mem359 »

I was working on code refactoring (delegating constructors, for after 0.4.7), and assigning default values to some member variables.

It can be annoying to figure out which header file the constants are hiding in. Some headers are hiding by being "included" by another header. EMPTY_STRING is defined in multiple places. Things like "extern ALL_EMPIRES" can be declared in several places.

I was thinking about smooshing the ones I am noticing into one header file, and then I only need to use that while refactoring.

1) Are there any design choices or compiler errors why I shouldn't do this reorganization (for a limited subset of default values)?
Since I am still new to C++ programs of this size, I might be unaware of a potential pitfall.

2) Preferences on which header file to use?
I could create a new one, but I was thinking about stuffing them into universe/Enums.h, which already has some of the useful default values in the enum lists.

User avatar
Oberlus
Cosmic Dragon
Posts: 5714
Joined: Mon Apr 10, 2017 4:25 pm

Re: Refactoring: move some of the constants into one header?

#2 Post by Oberlus »

Caveat: not a FO developer, didn't even see a piece of code.
Anyhows, my two cents:

Usually you want to keep constants and other definitions in the files they are more related. Something like (I'm making this up) definitons for species in the species headers. Thus, when changing something related to just one thing you only recompile the corresponding files, etc. If many relatively independent parts of the application include the same header and that header is changed then everything must be recompiled (also, for other kind of definitions, not just constants, that might make debugging a bit more labourious). Not a problem if you are sure nothing will change in that file for eons to come, but that may be hard to tell. Anyways, most IDEs provide tools to painlessly show/search definitions (functions, variables, constants...) and locate them quickly, so I wouldn't recomend to do that kind of refactoring unless those constants/enums make more sense in a header different than the current one.
(Edit: just ignore this if I'm missing the point)

User avatar
mem359
Dyson Forest
Posts: 214
Joined: Sun Jun 08, 2014 1:18 am

Re: Refactoring: move some of the constants into one header?

#3 Post by mem359 »

I should have mentioned, that the PR I'm working towards is aimed at making the code smaller by reducing the initializer lists or by using delegated constructors. Since I'm looking at a bunch of small changes in a lot of header and cpp files, it is inevitable that I will have to recompile everything. :twisted:

But I figured it might be a good time to reorganize/consolidate things like ALL_EMPIRE (default -1 for not assigned to a specific empire yet) and INVALID_OBJECT_ID, which I'm assuming have been stable in value for a long time.

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: Refactoring: move some of the constants into one header?

#4 Post by Geoff the Medio »

mem359 wrote:It can be annoying to figure out which header file the constants are hiding in.
Can you use a code editor that supports jump to definition / declaration for symbols?
EMPTY_STRING is defined in multiple places.
That's fine, because EMPTY_STRING doesn't need to be the same empty string object in all cases; it's usually just a file-scope constant to give functions something to return by reference. Any other file's EMPTY_STRING is not conceptually related to it (in contrast to ALL_EMPIRES);
Things like "extern ALL_EMPIRES" can be declared in several places.
The "extern" part is specifically saying that there is just one ALL_EMPIRES constant defined somehwere else, and to use that one to make sure they all stay consistent.
I was thinking about smooshing the ones I am noticing into one header file, and then I only need to use that while refactoring.
That's something to avoid, as it would mean that any change to that file or any of its contents would force recompilation of everything that includes it. Enums.h does basically what you're proposing for the general-use enumerations, and often has that issue.

User avatar
mem359
Dyson Forest
Posts: 214
Joined: Sun Jun 08, 2014 1:18 am

Re: Refactoring: move some of the constants into one header?

#5 Post by mem359 »

This is why I asked the question, instead of just doing it. :mrgreen:
I won't do this.
But let me toss this out, in case it changes anyones mind...
Geoff the Medio wrote:That's something to avoid, as it would mean that any change to that file or any of its contents would force recompilation of everything that includes it. Enums.h does basically what you're proposing for the general-use enumerations, and often has that issue.
But that is actually an argument for doing the change I am suggesting.

Consider something like UniverseObject.h, which has INVALID_POSITION and SINCE_BEFORE_TIME_AGE.
That file has had 8 commits over the last year.
Suppose those constants were moved to Enums.h.
Enums.h only had 3 commits over the last year.

Doesn't have to be in Enums.h, it was just a convenient file to pick.
(Maybe something like Export.h, which defines FO_COMMON_API, and has only changed once in 4 years.)
I could always create a new header, which has a just handful of frequently used (but never altered?) default constants. That file would change much less frequently than whatever header files currently have those constants.

As far as ALL_EMPIRES goes, the way it is used is more confusing for new programmers than it has to be.
ALL_EMPIRES is declared in 8 header files (including Empire.h), but it is defined on line 71 of Universe.cpp. Besides being buried in a non-obvious place, it is not declared in the Universe.h file. (It is declared in the UniverseObject.h file, which is included in the Universe.h file, which again is not helping the readability.)
Has its value ever deviated from -1? Or expected to, for some reason?
(I would think it would be simpler to stuff its definition into a header file.)

User avatar
adrian_broher
Programmer
Posts: 1156
Joined: Fri Mar 01, 2013 9:52 am
Location: Germany

Re: Refactoring: move some of the constants into one header?

#6 Post by adrian_broher »

> Are there any design choices or compiler errors why I shouldn't do this reorganization (for a limited subset of default values)?

It was done this way to reduce compile times without changing the file structure at all. Forward declarations were the easiest way to do that. Moving those forward declarations to files related by topic would pull in a cascade of unrelated headers. Moving those forward declarations into unrelated headers would make things even harder to understand. Creating new forward only headers is something to avoid in general unless you want to add the new files to all projects (which is quite a PITA, especially with the Xcode project if you don't have Xcode and it is still a bothersome bothersome task for Visual Studio).

> just ignore this if I'm missing the point

No Oberlus, you're quite right on how it should be. However every contributor must accept the fact that the FO codebase is 14 years old, changed the build system four times (Makefile, Autoconf, Scons, CMake + manual maintained Projects that will hopefully go away soon), replaced core dependencies multiple times (FMod -> OpenAL, ? -> OGRE + SDL1 -> SDL2, just to name a few) and just upgraded to a newer C++11 standard after a lot of tinkering. A codebase like this contains various kind of cruft, it's inevitable and sometimes you don't refactor things because it's not worth the effort before doing some other refactoring before that and one before that. Guess what I wanted to contribute in the first place and how much I achieved of that despite the fact that I'm in for four years now (hint: reusing better translation tools by utilizing the gettext library, achived so far: 0%).

> (Maybe something like Export.h, which defines FO_COMMON_API, and has only changed once in 4 years.)

What is the relation between 'ID representing all/any empire' and 'Macros to mark a symbol as public within a shared library'? (File X hasn't been touched for $TIMESPAN is not an argument for any programming design decision btw.)

> As far as ALL_EMPIRES goes, the way it is used is more confusing for new programmers than it has to be.

`extern` is a basic C/C++ concept. Should we avoid other basic language concepts like lambdas to avoid confusion for new programmers?

> Besides being buried in a non-obvious place, it is not declared in the Universe.h file.

Oh that place was obvious at some time when that line was added by a guy I never met 8 years ago. It just happend that the Universe class isn't in charge of the Empire ID-to-object mapping anymore and nobody considered moving it.

> Has its value ever deviated from -1? Or expected to, for some reason?

No it has not, but what is the point of your question? If you plan to replace constants with magic numbers or to 'consolidate' constants don't even bother to send a pull request. It will get rejected.

> (I would think it would be simpler to stuff its definition into a header file.)

Putting the definition into a header file would cause multiple definitions at link time.
Resident code gremlin
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz

User avatar
Oberlus
Cosmic Dragon
Posts: 5714
Joined: Mon Apr 10, 2017 4:25 pm

Re: Refactoring: move some of the constants into one header?

#7 Post by Oberlus »

As a newcomer, I agree with adrian and Geoff.

After having browsed the FO github for a few minutes, I can say this:

- The whole project is large enough for it not being readily understood (its structure) to any newcomer. That is true for any kind of big programming project, and for not so big too.

- The only way to get used to a project's structure is using it. Every (major) change intended to improve readability will disrupt all the older contributors and not help the new to come (only the one that is making that change).

- The distribution of definitions amongst the many files follows some (several) logics. There are different valid and reasonable ways to arrange things in a huge programming project with many concepts and files. For example, grouping all enumerations in a single file follows the homogeneity criterion (to put together things that have similar properties) but does not meet the functional criterion (to put together things that are related/dependent of each other). I usually favour functional over homogeneous criteria for code optimisation purposes (at least for 2012's compilers tend to do better optimisation over functions calls if they are in the same file), although that has little to do with the enums.h file if it has no functions in it.

- In the end, what really matters for readability when working in so big projects (that can't be keept into one's mind) is to use a good programming environment that easily let you see what is what. For example, I use Eclipse to program C, if I hover the mouse over a symbol (variable, constant or function, either call or declaration), it shows me a pop-up with its definition and where it is defined, and if I push F3 when cursor is in a symbol it automatically opens the file with its definition (or jumps to it if it's in the same file) for me.

All this is my opinion, my differ from the one of more profesional programmers.

defaultuser
Juggernaut
Posts: 854
Joined: Wed Aug 26, 2015 6:15 pm

Re: Refactoring: move some of the constants into one header?

#8 Post by defaultuser »

mem359 wrote:It can be annoying to figure out which header file the constants are hiding in. Some headers are hiding by being "included" by another header.
Most coding standards I've worked under have required that any needed headers be included at the lowest level that they are needed. If the headers are properly idempotent then including them more than once won't hurt.

Post Reply