Use Guild shared_ptr instead of raw pointers

[ramon-bernardo]

Pull Request Prelude

• [x] I have followed proper The Forgotten Server code styling.
• [x] I have read and understood the contribution guidelines before making this PR.
• [x] I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

• Replace guild raw pointers with shared_ptr.
• Use auto and const auto, at all, when possible.
• Added Guild::MEMBER_RANK_LEVEL_DEFAULT.

Discussion

Should we use shared_ptr<T> or T_ptr at all? GuildWars instead of GuildWarVector?

[MillhioreBT]

I definitely wouldn't remove the name GuildRank_ptr It would be nice to add Guild_ptr instead of using shared_ptr<T>() everywhere It's really horrible to see shared_ptr<T> a thousand times throughout the source code.

[Codinablack]

I definitely wouldn't remove the name GuildRank_ptr It would be nice to add Guild_ptr instead of using shared_ptr<T>() everywhere It's really horrible to see shared_ptr<T> a thousand times throughout the source code.

I'm the opposite on this topic. I feel that seeing std::shared_ptr() in the code, everywhere it's used, gives a very clear indication to whomever is reading it, what it is, without having to think about it, or take the time to get the compiler to show its underlying structure.

I also am opposite on the use of "auto" everywhere possible though too. I feel it should only really be used in certain situations, but for the most part, one should specify the type, so that whomever is reading it, know's what it is without having to think about it.

That's just my opinion though.

[EvilHero90]

The problem is rather if you have shared_ptr shattered across everywhere, if you ever wanted to change it to another pointer type you'd have to change every single occurency instead of just changing the superior declaration.

I'm also not happy with the declaration of variables inside if statements, we never did that before and we shouldn't start doing it now, it kinda makes the code unreadable just for decreasing the line size by 1 each time, not worth the trade

[MillhioreBT]

The problem is rather if you have shared_ptr shattered across everywhere, if you ever wanted to change it to another pointer type you'd have to change every single occurency instead of just changing the superior declaration.

I'm also not happy with the declaration of variables inside if statements, we never did that before and we shouldn't start doing it now, it kinda makes the code unreadable just for decreasing the line size by 1 each time, not worth the trade

for this there is the using keyword which is quite similar to typedef used to create alias

Although directly using std::shared_ptr in some cases is useful/fast and feasible, there are others in which it is not, for example having to use this in 100 different places, for that there is a better way to do it and that is to create an alias, this way the code becomes more consistent/legibility/maintainable

and whether or not people can recognize the type being used, I'm not sure but in almost all projects when an alias starts with the class name and an abbreviation of ptr, it implies that we are dealing with a smart pointer, At least that's what I've noticed, and in fact I like it that way.

Otherwise we will have more than 5300+ coincidences like in Canary

Just imagine that one day you want to change something, the name of the class, or maybe moder of unique or shared, anything you can think of... in that case your PR will have hundreds of changes, which are basically a replace XD

[ramon-bernardo]

I prefer shared_ptr<T> over T_ptr; it's more idiomatic and friendlier to the language. I tend to avoid using aliases when concealing types. As for maintenance, who would ever need to change shared_ptr to something else? It's unlikely to happen.

The use of auto is debatable, although nowadays it's generally preferable.

Initializing variables within an if statement is already common in many parts of TFS. Not only does it restrict the variable's scope, but it also checks for nullptr right away.

[MillhioreBT]

I prefer shared_ptr<T> over T_ptr; it's more idiomatic and friendlier to the language. I tend to avoid using aliases when concealing types. As for maintenance, who would ever need to change shared_ptr to something else? It's unlikely to happen.

The use of auto is debatable, although nowadays it's generally preferable.

Initializing variables within an if statement is already common in many parts of TFS. Not only does it restrict the variable's scope, but it also checks for nullptr right away.

Nowadays, who uses the notes blog to work on a project? The visual studio or visual studio code linter immediately shows you the type, and saying that something is unlikely to happen as an excuse to do things one way is not a good justification.

I could well say, well, since no one will change X thing, then I will add Y things...

Also, whatever the case, in TFS the smart pointers each have their alias and this is how it should be done, respect the code style of the repository

At least it is my personal opinion, if anyone else agrees or disagrees, please leave your opinion. ty

[ramon-bernardo]

I doubt anyone will test this pull request, and that's just how it goes. GitHub doesn't have a built-in linter, and this is where we review the code, LGTM. At this stage, using auto is irrelevant, as you need to know what each getT returns. Aliasing T_ptr achieves the same effect as auto.

Regardless, I'll adhere to the established pattern.

[ranisalt]

I'm 100% with @ramon-bernardo here. "if we ever have to change" OK but why would we ever? We probably have bigger fish to fry if we need to rename a class. Guild has that name for over 11 years now

About auto, everyone should probably always use it, as it helps avoiding lots of pitfalls related to conversion and casting, and I'll let Herb Sutter explain:

What does “write code against interfaces, not implementations” mean, and why is it generally beneficial?

It means we should care principally about “what,” not “how.” This separation of concerns applies at all levels in high-quality modern software—hiding code, hiding data, and hiding type. Each increases encapsulation and reduces coupling, which are essential for large-scale and robust software. (...) Hiding type (run-time polymorphism). Second, OO also gave us “separation of interfaces to hide type.” A base class or interface can delegate work to a concrete derived implementation via virtual functions. Now the interface the caller sees and the implementation are actually different types, and the caller knows the base type only—he doesn’t know or care about the concrete type, including even its size. The point, once again, is that the caller does not, and should not, commit to a single concrete type, which would make the caller’s code less general and less able to be reused with new types.

Also read answer 3 if it piques your interest.

[maattch]

Since this is now a shared pointer, i guess we dont need the delete this in Guild::removeMember anymore.

[ramon-bernardo]

@maattch thanks!

[ramon-bernardo]

After reviewing everything, it seems correct now. The last change for inline rank check

[ranisalt]

:rocket: