[I am inevitable]

Hi.

Reading code off the internet sometimes triggers me. I immediately start thinking: "hmm, why didn't you that, or that?". (I'm not trying to be that smartass or anything, and I'm by any means not the best, just wanted to make things a little clearer, okay, good).

Before we dive in, when writing something, always ask yourself these questions:
- What do I want to know?
- What do I already know?

Trust me, it's extremely helpful.

Here're a couple of dilemmas with examples, and more compact solutions:

1. A guy named Jerry wants a specific command to output the client's team. This is how he did it:

Spoiler

PHP Code:

#include <sourcemod>

#pragma semicolon 1

public void OnPluginStart()
{
    RegConsoleCmd("sm_test", Cmd_Test);
}

public Action Cmd_Test(int iClient, int iArgs)
{
    // Validate the client.
    if (0 < iClient <= MaxClients && IsClientInGame(iClient))
    {
        // If the clients team index is greater than two, i.e, if the client isn't a spectator.
        if (GetClientTeam(iClient) > 2)
        {
            // If the client is in the Counter-Terrorist team.
            if (GetClientTeam(iClient) == 3)
                PrintToChat(iClient, "Yay, you're in the Counter-Terrorist team!");
            
            // If the client is in the Terrorist team.
            else if (GetClientTeam(iClient) == 2)
                PrintToChat(iClient, "Yay, you're in the Terrorist team!");
        }
    }
    
    return Plugin_Handled;
} 


Now, this code is perfectly fine, but. We can shorten it... a lot:

Spoiler

PHP Code:

#include <sourcemod>

#pragma semicolon 1

public void OnPluginStart()
{
    RegConsoleCmd("sm_test", Cmd_Test);
}

public Action Cmd_Test(int iClient, int iArgs)
{
    // Validate the client.
    if (0 < iClient <= MaxClients && IsClientInGame(iClient))
    {
        // We assign the team index to a variable, to save some space.
        int iTeam = GetClientTeam(iClient);
        
        // If the clients team index is greater than two, i.e, if the client isn't a spectator.
        // We can then just use the ternary operator and we save lots of space!
        if (iTeam > 2)
            PrintToChat(iClient, "Yay, you're in the %sTerrorist team!", iTeam == 3 ? "Counter-" : "");
    }
    
    return Plugin_Handled;
} 


2. This one day I was writing a stats plugin and there was this one command that was supposed to create a menu, showing you the top players on the server, based different categories. Here's how I first did it:

Spoiler

PHP Code:

public void Menu_Top_Menu(int iClient)
{
    Menu hMenu = new Menu(Menu_Top_Menu_Handler);
    
    hMenu.SetTitle("Top Menu");
    
    hMenu.AddItem("", "Top Kills");
    hMenu.AddItem("", "Top Headshots");
    hMenu.AddItem("", "Top Noscopes");
    
    hMenu.Display(iClient, MENU_TIME_FOREVER);
}

public int Menu_Top_Menu_Handler(Menu hMenu, MenuAction hAction, int iClient, int iMenu_Param)
{
    if (IsValidClient(iClient))
    {
        switch (hAction)
        {
            case MenuAction_Select:
            {
                char sQuery[256];
                
                switch (iMenu_Param)
                {
                    case 0:
                        Format(sQuery, sizeof(sQuery), "SELECT * FROM user_stats ORDER BY exp DESC");
                    
                    case 1:
                        Format(sQuery, sizeof(sQuery), "SELECT * FROM user_stats ORDER BY kills DESC");
                    
                    case 2:
                        Format(sQuery, sizeof(sQuery), "SELECT * FROM user_stats ORDER BY headshots DESC");
                    
                    case 3:
                        Format(sQuery, sizeof(sQuery), "SELECT * FROM user_stats ORDER BY noscopes DESC");
                }
                
                g_hDatabase.Query(SQL_Top_Callback, sQuery, GetClientUserId(iClient));
            }
            
            case MenuAction_End:
                delete hMenu;
        }
    }
} 


And this too is just fine, but, we can be even more efficient with our lines, and here's the solution I found:

Spoiler

PHP Code:

public void Menu_Top_Menu(int iClient)
{
    Menu hMenu = new Menu(Menu_Top_Menu_Handler);
    
    hMenu.SetTitle("Top Menu");
    
    hMenu.AddItem("kills", "Top Kills");
    hMenu.AddItem("headshots", "Top Headshots");
    hMenu.AddItem("noscopes", "Top Noscopes");
    
    hMenu.Display(iClient, MENU_TIME_FOREVER);
}

public int Menu_Top_Menu_Handler(Menu hMenu, MenuAction hAction, int iClient, int iMenu_Param)
{
    if (IsValidClient(iClient))
    {
        switch (hAction)
        {
            case MenuAction_Select:
            {
                char sColumn[12];
                
                hMenu.GetItem(iMenu_Param, sColumn, sizeof(sColumn));
                
                char sQuery[256];
                Format(sQuery, sizeof(sQuery), "SELECT * FROM user_stats ORDER BY %s DESC;", sColumn);
                
                g_hDatabase.Query(SQL_Top_Callback, sQuery, GetClientUserId(iClient));
            }
            
            case MenuAction_End:
                delete hMenu;
        }
    }
} 


I believe that programming isn't just about making things work, it's about making it nice aswell.

With that being said, you can almost always make your code just a little bit better.

(Also, I don't even know if this page is useful, but, meh).

[Cruze]

Won't it be like this? Correct me if I'm wrong ._.

PHP Code:

PrintToChat(iClient, "Yay, you're in the %sTerrorist team!", iTeam == 3 ? "Counter-" : NULL_STRING); 


[I am inevitable]

Originally Posted by Cruze Won't it be like this? Correct me if I'm wrong ._. PHP Code: PrintToChat(iClient, "Yay, you're in the %sTerrorist team!", iTeam == 3 ? "Counter-" : NULL_STRING);

Woops, yeah

[Peace-Maker]

NULL_STRING has an undefined value, so you can't depend on it being an empty string. Better use an empty string "" right away!

[I am inevitable]

Originally Posted by Peace-Maker NULL_STRING has an undefined value, so you can't depend on it being an empty string. Better use an empty string "" right away!

Okay cool, I didn't know, ty ^^

[DarkDeviL]

Originally Posted by I am inevitable Now, this code is perfectly fine, but. We can shorten it... a lot:

It would have been even 'shorter', if you didn't create a new "iTeam" variable, and simply used:

like in the first example.


The ternary operator:

PHP Code:

iTeam == 3 ? "Counter-" : "" 


isn't always as useful, such as for example if the code you want to act based on the result of the condition is spanning more than one line, in these situations the classic if-then-else conditions may be more appropriate (and make the code more readable in the end).

[nosoop]

PrintToChat(iClient, "Yay, you're in the %sTerrorist team!", iTeam == 3 ? "Counter-" : "");

That is perfectly fine up until you want localizations (check out the Russian localization of team names on CS:GO), or different phrases depending on the team. At the very least I'd rather keep the entire team name in the third parameter to avoid these sorts of phrase concatenations.

Additionally, Menu_Top_Menu_Handler is in poor form -- the third parameter is not always going to be a client index, per the MenuAction documentation. You've gated off MenuAction_End, where the parameter in question is the MenuEnd reason, and so you're now always leaking handles (because the values that you think are clients are either 0 or negative). That said, I'd also take that approach to simplifying the query setup.

As far as clear code goes, I'm a fan of "Avoid Else, Return Early" to keep indentation to a minimum, but that's mostly a matter of opinion and not a strict style to adhere to.