Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modernize admincmd.sma #567

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

OciXCrom
Copy link
Contributor

@OciXCrom OciXCrom commented Sep 11, 2018

The changes include:

  1. Consistent variable names.
  2. Usage of new functions (read_argv_int, get_players_ex, create_cvar, etc).
  3. Translations for command syntaxes.
  4. Usage of %N instead of %s<%d><%s><> in log messages.
  5. Usage of %n instead of get_user_name where possible.
  6. Usage of %l instead of %L where possible.
  7. Consistent brackets, indentation, spaces and semicolons.
  8. Added missing translations and prefix for console messages.

@Arkshine
Copy link
Member

Arkshine commented Sep 11, 2018

  1. I really disagree with this. Hungarian notation how most of the people use it offers you no helpful information. It's unnecessary verbose and decreases readability (less fluent). A 'name' implies a string, why would you want to add 'sz', it's redundant. A variable name should be simple and meaningful. If you name it properly, you don't need a prefix. Like iSize, a size can be only an integer, what would want to add 'i', how it's helpful? It makes no sense. Just use descriptive variable name. The only prefix which could be okay is g_ for a global (even that, personnally I use an upper-case letter).

@IgnacioFDM
Copy link
Contributor

It's funny calling Hungarian notation "modernizing". I like g_ for globals. Not a fan of hungarian though.

Everything else, from the description (barely read the diff), seems good.

@WPMGPRoSToTeMa
Copy link
Contributor

  1. Usage of new functions (read_argv_int, get_players_ex, create_cvar, etc).

What about bind_pcvar_*?

@OciXCrom
Copy link
Contributor Author

@Arkshine I personally find prefixed variables easier to understand what and how they're used for. The variable type helps a lot in some situations, especailly when you have two separate variables for string and integer. Take for example the ban commands, they use a szMinutes and a iMinutes variable - in my opinion it's better than minutes_str and minutes_num. The majority of people use this coding style, so I think the default AMXX plugins should adapt to it. This is only my opinion however, if you don't like it, should I revert the variable names?

@WPMGPRoSToTeMa Yes, I can implement that too.

@Mistrick
Copy link

In my mind hungarian notation make sense for global variables, for local variables you just write more code.

https://github.com/Mistrick/MapManager/blob/ed09e4d78ab5c6cb617b2e37c06a08df2cea7648/mapmanager.sma#L1598
https://github.com/Mistrick/MapManagerModular/blob/14d88d41cf922a8b4cb2a5625330bf33406fe05f/addons/amxmodx/scripting/map_manager_core.sma#L507
For now I think second variant is more readable.

@OciXCrom
Copy link
Contributor Author

Anyways, we should decide which style to use. I see the style is mixed up in most of the plugins and I want to make it consistent.

@Arkshine
Copy link
Member

@Arkshine I personally find prefixed variables easier to understand what and how they're used for. The variable type helps a lot in some situations, especailly when you have two separate variables for string and integer. Take for example the ban commands, they use a szMinutes and a iMinutes variable - in my opinion it's better than minutes_str and minutes_num. The majority of people use this coding style, so I think the default AMXX plugins should adapt to it. This is only my opinion however, if you don't like it, should I revert the variable names?

I can't find the example you're talking about, show me some code.
If you have to "separate", this means you name the variable wrongly or is not descriptive enough in the context. As simple as that. The code should be read as you would read English. Suffixes make it harder. People will always follow blindly what they see without thinking, like they're doing with the engine -> fakemeta conversion, right (in my very first plugin, I did not know how to name, I used sz on string because I saw others people doing that). I won't allow this, it's really ridiculous.

@OciXCrom
Copy link
Contributor Author

Lines 321 and 333 in my modified admincmd.sma. So, which style do you suggest using exactly? The default code has prefixes on some places and doesn't in other.

g_global_variable_name or g_GlobalVariableName or g_Global_variable_name?
local_variable_name or localVariableName?

@Arkshine
Copy link
Member

This is not a good example, because here you could use directly read_argv_int.
That's said, you check first the context, it's about retrieving values from a command's arguments.
I would probably use new minuteArgument[32] and new minute. Variant could exist. The point is just to try to name based on the context.

new szMinutes[32];
read_argv(2, szMinutes, charsmax(szMinutes));

new iMinutes = str_to_num(szMinutes);
new minutesArgument[32];
read_argv(2, minutesArgument, charsmax(minutesArgument));

new minutes = str_to_num(minutesArgument);

The latter is easier to read and less confusing.

@OciXCrom
Copy link
Contributor Author

@Arkshine I reverted the variable names.

@Daniele386
Copy link
Contributor

German translation is partially outdated, maybe you want to include possible changes from #620?

@OciXCrom
Copy link
Contributor Author

@Daniele386 I'll do it when the PR gets merged.

Copy link
Contributor

@Daniele386 Daniele386 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -91,10 +123,10 @@ PERM = für immer
CLIENT_BANNED = Spieler "%s" gebannt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FOR_MIN = für %d Minuten

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no param for %d in the .sma file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought at line 89?
In the other languages you changed %s to %d?

Copy link
Contributor Author

@OciXCrom OciXCrom Oct 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 89 doesn't even have any %s/%d parameters.
I didn't change any %s to %d.

Copy link
Contributor

@Daniele386 Daniele386 Oct 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I blind?

I'm talking about lines

  • 89 (in your version line 121)

there is "%s"

and there are "%d":

  • 12
  • 164 (in your version line 196)
  • 239 (in your version line 271)
  • 314 (in your version line 346)
  • 389 (in your version line 421)
  • 464 (in your version line 496)
  • 539 (in your version line 571)
  • 614 (in your version line 646)
  • 689 (in your version line 721)
  • 764 (in your version line 796)
    and so on...

But you haven't change the .sma in that point, have you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not blind. FOR_MIN takes now an integer, so it should be %d.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I thought you meant line 89 in my version. I'll do the changes soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants