alliedmodders / amxmodx

AMX Mod X - Half-Life 1 Scripting and Administration
http://www.amxmodx.org/
477 stars 198 forks source link

Update admin.sma #1063

Open mlibre2 opened 1 year ago

mlibre2 commented 1 year ago

changelog

added

edited

replaced

eliminated

luxxxoor commented 1 year ago

you have wrong indentation, also why static instead of new ? you keep memory used instead of freeing it

mlibre2 commented 1 year ago

you have wrong indentation, also why static instead of new ? you keep memory used instead of freeing it

  1. wrong indentation? #1073
  2. static or new? https://forums.alliedmods.net/showpost.php?p=344418&postcount=1
luxxxoor commented 1 year ago

you should only use static when its not feasible to use new

for example:

1.a function that is called multiple times in a second 2.you make a heavy operation and you don't want to repeat it every time

yes, static is faster than new, but you keep memory occupied, why would you need to keep memory occupied for a function that is call only once in a minute ?

just think why c/c++ or similar languages dont use global variables and instead prefer to use dynamic allocation

mlibre2 commented 1 year ago

is relative, the same has been previously applied here:

rtxa commented 1 year ago

Well, memory is cheap today and static is useful in expensive operations, but still I see this PR trivial or really minimal priority because the changes will not bring any benefit because this code isn't doing anything expensive or not being called so many times. In any case, the use of constants instead of magic numbers is a great change and isn't mentioned in the PR.

Also, I see that swapping console_print() to engclient_print() is not a good change because the former has support for UTF-8 character which is useful in a multilingual context while I'm not that sure about engclient_print(). So by trying to bring a small optimization, you're maybe breaking something. I have notice too that the plugin isn't fully translated.

Other reason which the PR is not good is whitespaces changes and code formatting changes which obsfucates what you're really trying to fix/improve and less chances to get your PR merged. Those should be in another PR or in the final commit.

mlibre2 commented 1 year ago

well this is just the tip of the iceberg, to update all other plugins!