How is my code?

[SupSuper]

This is another general feedback question for all the curious programmers and developers who have poked around my codebase.

One of my biggest issue with open-source projects has always been peeking at the code and feeling like I was staring deep into the mouth of Cthulhu or something. I'm sure every programmer feels like that when looking at someone else's code.

Still, it got me curious. How do people feel about my code? After all, I hope people can make use of it for their own purposes. Is it friendly? Is it readable? Is it clean? Is it the scariest thing you've ever seen? Be honest.

[michal]

I've only peeked at your code several times, but it looks nice and clean.

My only concern is many classes in one directory.

Maybe source could be spplited into several directories / namespaces, like:

Code: [Select]

src/
  draw - some low level drawing things like Surface, PolyLine, Polygon, Pallete
  gui/ - classes like Bar, ArrowButton, Currsor
  geo/ - states related to geoscape / interception
  base/ - states related to base
  rules/
  resources/

or something like that

[SupSuper]

I do keep the code organized, but only at the IDE-level:

I guess you don't get to see it if you're not on Windows.

Using real folders/namespaces would probably make more sense, but I don't like the idea of having to keep track of it all just to include a file. I guess I'll have a look at how other projects deal with it.

[n0n3]

Code is clean, well commented - it's easy to read and understand even for a beginner like me.
And I agree with michal that it could be in different directories it would be clearer for people with vi

[pmprog]

Not looked at it for a while, but from what I remember it was very good. I was/am impressed.

I work in VS2008, so I got the folder organisation too,  so that didn't bother me :-)

I need to get around to downloading the latest SVN code and seeing where you're up to.

[sir_nacnud]

Overall the code is pretty good, very easy to read.  I also think architecturally it is very sound.

There are a couple of areas that could use improvement.  These aren't anything major, just small things.

• Passing objects by value to functions - Most places in the code base pass std::string objects by value.  This causes extra copies of the objects to be made.  These can easily be replaced by a const  reference, ie "const std::string&"
• Including "using namespace std" in header files - This is usually not a good idea to use "using namespace x" in a header file.  This forces anyone who includes the header file to use that namespace.
• Use const on getter methods - It is a good idea to make getter methods const, this prevents member variables from being modified.  For example, if you had a method "Target* getTarget()", it would change to "Target* getTarget() const"
• Hard coded game data - I saw a couple of classes that had game data hard coded, example XcomRuleset.  This may be to due to the current state of the project, I would think eventually you would want to clean this up.
• Code duplication - I saw a couple of places where code was copied and pasted with a few things changed.  Example Globe::cachePolygons(), same logic copied and pasted, just the data structures being operated on were different.  It would be better to create a function instead of duplicating the code.

Keep in mind these are suggestions that I think would make the code base better and I would gladly help out with any of them.

[pmprog]

• Use const on getter methods - It is a good idea to make getter methods const, this prevents member variables from being modified.  For example, if you had a method "Target* getTarget()", it would change to "Target* getTarget() const"

I don't quite understand this one. The compiler already won't let you run code like "getTarget() = &DummyTarget".

[SupSuper]

I try to keep the code clean and standardized, but if there's something I've learned, standards/best practices are hard and ambiguous and confusing and with no consensus, specially when you work with multiple languages, and learning them all would take forever. So I just try to stay on the sensible side. Clean code is nice, but if I start worrying too much about that kind of stuff I just bog myself down when the player doesn't care about any of that, they just want features.

Also when I first started the project I was working with Java at university so there's probably a lot of influence there. One class per file, getter/setters, camelCase, _members, etc. I'm sure that makes C++ gurus break out in hives, but I've kinda grown fond of the style.

Still if you wanna improve the code, be my guest. If it's sensible and consistent and easy to maintain, I'll try to adopt it. As long as I don't end up losing sight of my own code.

To be more specific:

Passing objects by value to functions - Most places in the code base pass std::string objects by value.  This causes extra copies of the objects to be made.  These can easily be replaced by a const  reference, ie "const std::string&"

Well I tend to use strings like simple types (int etc.), won't I end up accidentally changing an object's string when I don't want to? Like:

Code: [Select]

string stuff(const string &s)
{
  s = "lalalala";
  return s;
}


Including "using namespace std" in header files - This is usually not a good idea to use "using namespace x" in a header file.  This forces anyone who includes the header file to use that namespace.

Fair enough, I just can never remind myself to put std:: everywhere.

Use const on getter methods - It is a good idea to make getter methods const, this prevents member variables from being modified.  For example, if you had a method "Target* getTarget()", it would change to "Target* getTarget() const"

Isn't that a bit restricting? Like requiring all related variables to be const or never changing or something? Gawd knows I probably have some half-assed code buried in there somewhere that might break.

Hard coded game data - I saw a couple of classes that had game data hard coded, example XcomRuleset.  This may be to due to the current state of the project, I would think eventually you would want to clean this up.

This will be moved to external files later on when time permits.

Code duplication - I saw a couple of places where code was copied and pasted with a few things changed.  Example Globe::cachePolygons(), same logic copied and pasted, just the data structures being operated on were different.  It would be better to create a function instead of duplicating the code.

Every once in a while I get lazy or tired, and prefer to just get something done rather than doing it all neat and tidy. After all it's still just my code, it probably won't get all spread around and used by various dangerous people like a library, so I can afford to be a bit lenient. If it's really bad I usually go back to it later to clean it up though. Although the Globe is pretty much one of the biggest ugliest classes and I never enjoy poking it.

I don't quite understand this one. The compiler already won't let you run code like "getTarget() = &DummyTarget".

I think a const method stops the implementation from changing the object and thus prevents mistakes like "getTarget() { _target = 1000; }". Don't quote me on that though.

[sir_nacnud]

Passing objects by value to functions - Most places in the code base pass std::string objects by value.  This causes extra copies of the objects to be made.  These can easily be replaced by a const  reference, ie "const std::string&"

Well I tend to use strings like simple types (int etc.), won't I end up accidentally changing an object's string when I don't want to? Like:

Code: [Select]

string stuff(const string &s)
{
  s = "lalalala";
  return s;
}


I can understand coming from java and wanting to use a string as a primitive type.  In c++ so you want to treat them like you do other objects.
By making the s parameter const, you can't modify it.  The const reference makes the variable "read only" and avoids the copy you would have if you did pass by value.  If you want to modify the parameter, just make it a regular pass by reference. 

It is probably not a good idea to "reuse" a parameter that is pass by value after you are done with it.  You don't really gain anything verses declaring one within the scope of the function.  Also it may make the code harder to understand.

Use const on getter methods - It is a good idea to make getter methods const, this prevents member variables from being modified.  For example, if you had a method "Target* getTarget()", it would change to "Target* getTarget() const"

Isn't that a bit restricting? Like requiring all related variables to be const or never changing or something? Gawd knows I probably have some half-assed code buried in there somewhere that might break.

The const functions are more restrictive.  A const function requires that none of the class member variables are modified during the function.  The other restriction is that only const functions are called within the function.  You wouldn't need to change the type of your class members.  Generally if you just have a simple getter method that is  returning a member variable, you can easily change the function to be const.

[wITTus]

Use const on getter methods - It is a good idea to make getter methods const, this prevents member variables from being modified.  For example, if you had a method "Target* getTarget()", it would change to "Target* getTarget() const"

Isn't that a bit restricting? Like requiring all related variables to be const or never changing or something? Gawd knows I probably have some half-assed code buried in there somewhere that might break.

The const functions are more restrictive.  A const function requires that none of the class member variables are modified during the function. [...] Generally if you just have a simple getter method that is  returning a member variable, you can easily change the function to be const.

And you'd better return a const type as well. Because the caller may access and modify the returned "Target"-object which is a member of your class which... therefore may get modified.

So your "getTarget() const" statement wouldn't be so true anymore (you're lying: there's no guaranteed constness for your private data), since this function indirectly may modify a member due to providing non-const access to others.

It would be a way better option to leave the function non-const in order to indicate that, by calling the getter function, the class may indeed be modified.

See this example: https://codepad.org/spLX9mGG

Basically, by storing pointers, you get indirect ownership and therefore constness for your pointers, but not for the data pointed to. Generally, you wouldn't get away with this: https://codepad.org/OlCMvjdg

Btw, getters and setters aren't a good idea anyway.

There is a reason that we keep our variables private. We don't want anyone else to depend on them. We want the freedom to change their type or implementation on a whim or an impulse. Why, then, do so many programmers automatically add getters and setters to their objects, exposing their private fields as if they were public?

[Yankes]

And you'd better return a const type as well. Because the caller may access and modify the returned "Target"-object which is a member of your class which... therefore may get modified.
So your "getTarget() const" statement wouldn't be so true anymore (you're lying: there's no guaranteed constness for your private data)

pointer is member not object that is pointed by that pointer. "getTarget() const" mean only that "this" cant be modified.
but your point is true when "Target"-object belong only to our object ("this") and everybody outside is forbidden to edit this when our object is const.

[sir_nacnud]

Use const on getter methods - It is a good idea to make getter methods const, this prevents member variables from being modified.  For example, if you had a method "Target* getTarget()", it would change to "Target* getTarget() const"

Isn't that a bit restricting? Like requiring all related variables to be const or never changing or something? Gawd knows I probably have some half-assed code buried in there somewhere that might break.

The const functions are more restrictive.  A const function requires that none of the class member variables are modified during the function. [...] Generally if you just have a simple getter method that is  returning a member variable, you can easily change the function to be const.

[
And you'd better return a const type as well. Because the caller may access and modify the returned "Target"-object which is a member of your class which... therefore may get modified.

So your "getTarget() const" statement wouldn't be so true anymore (you're lying: there's no guaranteed constness for your private data), since this function indirectly may modify a member due to providing non-const access to others.

It would be a way better option to leave the function non-const in order to indicate that, by calling the getter function, the class may indeed be modified.

See this example: https://codepad.org/spLX9mGG

Basically, by storing pointers, you get indirect ownership and therefore constness for your pointers, but not for the data pointed to. Generally, you wouldn't get away with this: https://codepad.org/OlCMvjdg

Btw, getters and setters aren't a good idea anyway.

There is a reason that we keep our variables private. We don't want anyone else to depend on them. We want the freedom to change their type or implementation on a whim or an impulse. Why, then, do so many programmers automatically add getters and setters to their objects, exposing their private fields as if they were public?

The idea of making the function const is not that the return value should be const, but that the pointer stored in the class can't be modified or any other member variables for that matter.  Making the function const is separate from the return value being const.  I agree having a pointer returned that you can modify the object is generally not a good practice.

Also, why do you think getter/setters are bad?  You prefer making those member variables public?

[wITTus]

I know that it makes this become const and I know that the pointer itself gets const, but well, you both got my point anyway. I was rather speaking about the general aspect of constness of functions within a class and its true meaning¹, rather than about the syntactical correctness.

For example, I as a library user might see that a function is declared const. So as logical consequence I'm asserting that this function won't modify my object in any way. But if I am able to modify its private variables² due to the exposure of its private pointers (through a getter), my object may change its state and behave differently after that call, which can be fatal, not only in a multithreaded environment.

[SupSuper]

I don't see why I shouldn't have getter/setters for values that are *just values*, there's no deep logic behind them, they don't contain state or whatever, the class will handle the value as it sees fit. In your example, the speed can change in any moment however it wants, and your alternative would just have me duplicate every single operator like:
plane.addSpeed
plane.removeSpeed
plane.stop
plane.etc.

And classes interact with one another, they're not black boxes so getters are inevitable, even if it makes stuff like const ambiguous (but none of my pointer getters guarantee the class won't be changed anyways, they just guarantee the *pointer* won't be changed).

[DaiShiva]

Code: [Select]

plane.addSpeed
plane.removeSpeed

would be:
plane.addSpeed(10)
or
plane.addSpeed(-10)