Quote:
Originally Posted by Dracosto
Sir its been years and i find this work stable and no more rooms to improve, therefore the plugin is hereby concluded as Final. Can you review this now to whether approve it or not?
|
Approvers aren't very active lately, plugins rarely get reviewed.
I took the liberty to personally look at the code and found a lot of things that can be improved and approvers would probably tell you to do so. Here's some of them:
Code:
new has_spawned[32]
I'm assuming this is a player id. If so, this will output a error and the connected functions won't work if called on the 32nd player. The array size should be 33 (MAX_PLAYERS + 1), not 32.
------------------
Code:
set_task(1.0,"regeneration",current_king,"",0,"b")
set_task(1.0,"NoDrown",current_king,"",0,"b")
As these are set in plugin_init() and "current_king" is a dynamic variable, the task id is useless as it will be 0.
------------------
Code:
register_concmd("kingnoclip", "NoClip",0,
You should use predefined constants (ADMIN_ALL) instead of magic numbers.
------------------
Code:
get_modname(mod_name,31)
No reason to hardcode the buffer lengths. Use charsmax().
------------------
Code:
console_cmd(0,"mp_friendlyfire 0") // Disables FF on Map
console_cmd(0,"mp_autoteambalance 1") // Enables Team Balancer on Map
server_cmd("sv_maxspeed 9999") // Infinite Max Speed on server
Why console_cmd() for some and server_cmd() for others? Probably your best bet is using set_cvar_num() instead.
------------------
RGB format can't be more than 12 characters.
------------------
Code:
get_pcvar_string(cvar_mincolor, colors, sizeof colors - 1)
Getting cvar values in plugin_init() will always get the original value set in the .sma file. You should get them in plugin_cfg().
------------------
Code:
public plugin_precache()
{
if(czero_running)
{
if(get_pcvar_num(cvar_skin)==1
Again, getting values in plugin_precache() has no effect.
------------------
Code:
precache_model("models/player/king_t/king_t.mdl")
precache_model("models/player/king_t/king_tT.mdl")
precache_model("models/player/king_ct/king_ct.mdl")
precache_model("models/player/king_ct/king_ctT.mdl")
Hardcoded paths, not a good idea in 2021.
Also you should use precache_generic() for player models as they are set client-side. Using precache_model() uses up precious resource space that adds up to the 512 limit for no reason.
------------------
Code:
register_event("ResetHUD","user_respawn","b")
This is so 2005. Use "Ham_Spawn" to hook player spawn.
------------------
Code:
#if defined noclip_included
precache_sound("misc/king_noclip.wav")
#endif
Why hardcoded defines instead of cvars?
Also, this sound in particular isn't used anywhere.
And again, hardcoded paths.
------------------
Code:
new ConfigDir[64],FullDir[128];
get_configsdir(ConfigDir,charsmax(ConfigDir));
formatex(FullDir,charsmax(FullDir),"%s/king_of_the_hill.cfg",ConfigDir);
You can use 1 less variable by doing this:
Code:
new FullDir[128];
get_configsdir(FullDir,charsmax(FullDir));
add(FullDir,charsmax(FullDir),"/king_of_the_hill.cfg");
------------------
Code:
server_cmd("exec %s/king_of_the_hill.cfg", ConfigDir)
Why? You already have the full path formatted in "FullDir".
------------------
Code:
if(current_king == 0)
{
while(current_king == 0 || get_user_health(current_king) == 0 || has_spawned[current_king] == 0)
{
current_king = random_num(1, inum)
}
get_user_name(current_king, name, 32)
set_hudmessage(0, 100, 200, 0.05, 0.50, 2, 0.1, 4.0, 0.02, 0.02, 10)
show_hudmessage(0, "[KING OF THE HILL] The new KING is %s", name)
console_print(0, "[KING OF THE HILL] %s is the New King.", name)
}
Indentation is gone.
What would happen when the server is empty? Infinite loop or even an error because it would be random_num(1, 0)!
------------------
Code:
new idx[1];
idx[0]=current_king;
No reason to pass this to a task when "current_king" is a global variable.
------------------
Code:
set_task(get_pcvar_num(cvar_gduration) * 1.0, "ungod", 0, idx, 1); // We put * 1.0 to do the float and turn to num
get_pcvar_float()?
------------------
Code:
if(get_user_health(current_king)!=get_pcvar_num(cvar_maxhp))
What if the health is more than the cvar?
------------------
Code:
public Damage(current_king)
{
if (!is_user_alive(current_king) || !gain[current_king]) return PLUGIN_HANDLED;
Return values have no effect with register_event().
------------------
Code:
new deadone[32]
new badone[32]
if (unlucky_one == current_king)
{
No reason to create the variables before checking if you need to use them.
------------------
Code:
public PreThink(id)
{
new buttons = pev(current_king, pev_button);
new flags = pev(current_king, pev_flags)
if(czero_running)
{
if(get_pcvar_num(cvar_skin)==1 && file_exists("models/player/king_t/king_t.mdl")==1 && file_exists("models/player/king_ct/king_ct.mdl")==1)
{
if (get_user_team(current_king) == 1) cs_set_user_model(current_king, "king_t")
if (get_user_team(current_king) == 2) cs_set_user_model(current_king, "king_ct")
}
Hell, no! Why would you set the model 100 times per second instead of once when the player spawns?! This not only incredibly resource-heavy, but breaks compatibility with any other plugin that changes player models. Same goes for everything else you're doing in PreThink(). You should only use this forward when there's no other viable solution and this does not apply for anything you're doing here.
------------------
Code:
public reset_spawn()
{
for(new i = 0 ; i < 33 ;++i)
{
has_spawned[i] = 0
}
}
arrayset(has_spawned, 0, sizeof(has_spawned))
Also, this should be a bool.
As you can see, there's quite a loot of room for improvement and this was only a quick glance at the code. The code is far from an "approval state" in its current condition according to nowaday practices.
__________________