[Refactor] Auto is overused in the entire codebase

[sfc-gh-rmaj]

auto keyword is overused in the entire codebase. As per Google C++ Style Guide type deduction should be used to make the code clearer and it's not a case in forgottenserver (example. Most of the auto usage can be refactored to explicit type. It will have positive impact on readability and help new contributors with getting a grasp in the code.

[Shawak]

auto casterPos = getNextPosition(dir, targetPos); yeah would could that be.. maybe a.. position?

Strongly disagree.

[ArturKnopik]

For me is clear with type will auto hold in your example
Also strongly disagree.

[sfc-gh-rmaj]

Thanks for replies!

Could you elaborate more on why the use of auto is justified here? Also why you don't agree with the Google C++ Style guide? There are much more examples in the codebase and clearly some of them aren't trivial.

Especially in open source projects writing explicit types may help ramp up engineers who would like to hop on and contribute.

[Shawak]

Well first of all typing explicit types everywhere emakes the code harder to read, and thats why most experienced cpp devs are actually against that. Also did you even read the style guide yourself?

The fundamental rule is: use type deduction only to make the code clearer or safer, and do not use it merely to avoid the inconvenience of writing an explicit type. When judging whether the code is clearer, keep in mind that your readers are not necessarily on your team, or familiar with your project, so types that you and your reviewer experience as unnecessary clutter will very often provide useful information to others. For example, you can assume that the return type of make_unique() is obvious, but the return type of MyWidgetFactory() probably isn't.

Consider your example:

auto casterPos = getNextPosition(dir, targetPos);

Is imo way easier to read than this:

Position caster = getNextPosition(dir, targetPos);

In a lot of cases it is always easier to just include the type in the variable name or if it is obvious just dont type it out at all.

[sfc-gh-rmaj]

Why would I send something that I haven't read? It seems like a bit unneded passive agressive like comment. At the end we all want to have code that is easier to develop and easier to read. Also let's say I was kinda required to know this style guide by heart :).

The part you quoted in my opinion suggest that auto is unneeded here. As a first read you don't know the type without going to getNextPosition function. Is it Position, Pos or PositionSomething? For you it's clear for a first time reader it's not.

[...] do not use it merely to avoid the inconvenience of writing an explicit type

I think that's exactly the case here.

Also I wouldn't suggest to change the name of the variable as you did as it's misleading then in the rest of the function. I would stay with the previous name making it:

const Position casterPos = getNextPosition(dir, targetPos);

(even adding a const qualifier here as it's also not used enough).

It's 4 characters more in type. Is it that much more? You get benefit of instantly knowing the type. Here it's obvious, but is it in the rest of the examples from repository? I wouldn't be so sure. Also the usage of auto is inconsistent as in the same file there is this line. Unifying those will improve the readability by a lot.

Well first of all typing explicit types everywhere emakes the code harder to read, and thats why most experienced cpp devs are actually against that.

That's not true as Google developers are actually against using auto that much (as style guide suggest). Also I am not proposing to write explicit types everywhere.

[Shawak]

After giving it a second thought, I think I kinda agree with you. While it's not a good idea to compare twelve year old code with modern standards, it's probably a good idea to replace auto at certain points with actual types given the scale and type of this project.

But to be honest, it might be easier to just enable type hints in your IDE. Also, I believe that this would harm contributing if it became a requirement for PRs.

[sfc-gh-rmaj]

It's actually a great way to get new contributors who can do something useful and try to compile the project.

[Codinablack]

It's actually a great way to get new contributors who can do something useful and try to compile the project.

That's an outrageous statement. There is absolutely not requirement on this particular issue as of now, so anybody, anywhere can submit any code they feel like without fear of it being rejected from using auto or not... enforcing it to be one or the other, only limits the amount of contributors who would naturally conform or willingly write and conform to such a standard.

So I have to ask you, how could enforcing strict rules for this one way or the other possibly "get new contributors"?

@topic I used to love the explicit typing myself, but as I have grown so has my preference, and the use of auto does indeed reduce the clutter many times, making the code more "readable". As for not knowing what the type is, most IDE's do indeed tell you if you hover, but they all allow you to see the definition of the function being called that's return is being assigned to, and any other case where there is not a function returning, it becomes more clear what the type actually is for the variable being assigned.

So I have to say I disagree with the idea of enforcing this one way or the other, completely. If anything, The only place I absolutely prefer explicit typing is as argument variables (ie in the signature), and class members. Otherwise, I say auto should be used as much as possible.

[sfc-gh-rmaj]

Hey, thanks for replay. You have a large refactoring here which will touch multiple files. Mostly you want to do it file per file, or even divide it into smaller chunks. That's when new contributors make a small change with refactoring auto use, compile the project and submit PR. Seems like cool ramp up task. Also you are anyway following Google C++ style guilde as stated in TFS Coding Style Guide, so auto usage should be limited as stated there.

[Sorky96]

Please feel free to correct me if I’m mistaken. In your first message, you provided an example of how auto should be used—was that aligned with best practices? Or am I misunderstanding something?

auto can and should be used within functions, but it generally should not be used for class member variables.