AMX Mod X Plugin Approver
Join Date: Sep 2013
This mod seems a nice ideea, and probably more complete than others freeze tag plugins around here. Anyway, here are some improvements that you could do:
- I would not hardcode the configurations inside the plugin, you can make cvars, much better for the end-user, easy to edit, etc. If for some reasons configuration should not be changed in-game(could break gameplay for example) cache the cvar. It would still be better than hardcoding the config, it will no longer requiere the plugin to be recompiled.
- Some stuff may not be exposed to the end user. I mean, values like MAX_CLIENTS or SBAR_STRING_SIZE should not be tagged with "options" keyword, this will make people think they can edit it. This is not something which should be changed by someone unexperienced.
- static const Float:vecZero = any specific reason for static ? It’s already global. Same for g_pResultDummy
- idattacker < 1 || idattacker > g_iMaxPlayers you can do like !(1 <= idattacker <= g_iMaxPlayers). Just a hint.
- I see that you started to support 1.8.3, and it provide a define for MAX_PLAYERS, so check amxx version and use it instead if it's the case.
- On the CBasePlayerItem offsets drop the stock keyword. Everything that is inside the plugin should be used. No point in using stock
- If I remember well, the last param in ArrayCreate does not work as it should. It is used to create empty entries into the array, so you can use ArraySet/ArrayGet on them, but it won’t until you push into this cells. As of AMXX 1.8.3, ArrayResize() exists for this purpose
- Destroy the trie handles in plugin_end, this is a good practice and also make sure you won’t waste memory. This is true for all handles, for amxx < 183. In 1.8.3 memory is cleaned exactly when it should.
- Style 1 of natives is deprecated, use style 0. I see no reason to mix natives with style 1 and 0, just use style 0, because 1 is deprecated.
- In your natives, checking if "id" is actually a valid player may be a good ideea.
- Do not hardcode array size, you can use sizeof to get it. Just picking a random example: memset(_:g_flLastOrigin[ i ], 0, 3)
- Checking only id > 0 is not enough, you should check if it is between 1 and MaxPlayers given that this index come from a native and the coder my give you an id bigger than 32.
- Since you use a dynamic array, what is the reason for limiting the item count ? This would make sense only if you are using a normal one with a limited size.
- In MenuShop static should not be used. Even if the array has 1024 cells, how many times per round will a player buy something in order to cause a stack error ? Well, you could let static on szBuffer, but remove it from szItem.
- If you reset values on client_disconnect should be enough. Doing it again in client_connnect does not make sense. Edit: Lol, or maybe yes. I saw that 1.8.3 fixes a bug in which client_connect won't be always paired with a client_disconnect forward call. So they added a new forward called client_disconnected and deprecated client_disconnect.
- Do not loop players as you do, from 1 to MaxPlayers. Use get_players for several reasons:
- It's faster
- It will give you only valid players
- Allow you to filter by flags(alive, dead, team, bots and much more)
- Same story, in CBasePlayer__Killed static is not needed.
- Be consistent, in CPointEntity__IceCubeThink everything should be static.
- flCurrentTime = get_gametime() should be done before set_pdata_float, so you could use flCurrentTime there too.
- Don't use HAM return constants in an engine forward. Use them only in ham forwards.
- When sending important messages on which gameplaye is based I would not use unreliable message, this could fuck up the gameplay. It's true that reliable ones might drop the client from server, but it is not likely to happen if messages are send correctly and you don't set 100 of them. Also, reliable messages are guaranteed to be send to players.
- Are you sure you need 0.01 as think time ? It seems too often.
- PreThink is called a lot of times on each frame, so if player has 100 fps, then prethink will be called 100 times/second. Everything here should be static. What you can do is to unregister the forward when g_pPlayerInfo[ id ][ Player_ProtectTime ] become 0, so you avoid unneded calls.
- Please do not hardcode path, you have natives to retieve them. This is something which can be easely changed from core.ini.
- In UTIL__GetWeaponByKiller checking if entity private data is fully initialized is useless, you do it in the kill forward anyway.
- You should be consistent when naming your variables, forwards, etc. For example, in some you name the hooks by their base function name(CBase_*) and in some place just give a random name.
Personal opinion, this is not required, but this is how I would do it.
- I never played this mod, but I think it would be nice to make a configuration file in which the user can edit the model, the sound and the sprites.
- If you want to have a more organised code, you could use only one array instead of 3 and make an enumeration(name, cost, team). I am not sure how much memory would you save by doing so(if any), but IMO it will be more clean.
- Do not create a variable if you will use it one time. Well, this is not dramatic, and could improve readability, but here it is not the case.
Can work directly with ArrayGetCell
new iCost = ArrayGetCell(g_pExtraItemCost, iItemId);
g_pPlayerInfo[ id ][ Player_Money ] -= iCost;
- You are doing initializations when there is absolutely no reason for them. For example:
It may be only my personal taste, but I consider it unneeded.
new CsTeams:teamID = g_pPlayerInfo[ killer ][ Player_Teamid ];
- Setting m_bMapHasBuyZone offset could be done in HLTV event. But it may be ok as you do it too.
- Instead of using the __coordOffsets list you can just randomly search around player for empty position, and once you found it set origin and stop searching. What I mean is to create a range, let’s say +-10 around current origin and get a random float from it. This is just an another algorithm ideea.
Huh, quite big plugin. Sorry if something is messy. I felt like this is enough for now.
Last edited by HamletEagle; 08-31-2015 at 12:00.
Reason: Got more technical infos from Arkshine, so some things are changed.