[Tutorial] Bad scripting habits - What not to do in a script

[Tutorial] Bad scripting habits - What not to do in a script - Printable Version

+- SA-MP Forums Archive (https://sampforum.blast.hk)
+-- Forum: SA-MP Scripting and Plugins (https://sampforum.blast.hk/forumdisplay.php?fid=8)
+--- Forum: Scripting Help (https://sampforum.blast.hk/forumdisplay.php?fid=12)
+---- Forum: Tutorials (https://sampforum.blast.hk/forumdisplay.php?fid=70)
+---- Thread: [Tutorial] Bad scripting habits - What not to do in a script (/showthread.php?tid=247454)

Bad scripting habits - What not to do in a script - ~Yoshi - 09.04.2011

I wrote this for new scripters, or old scripters who have bad scripting habits.
Most things are about keeping the code orginized, some to make the code more effective.

Not Using arrays or loops

Instead of using a lot of integers, it's shorter and more effective to use arrays and loops.

Bad example:

pawn Code:

public OnPlayerConnect(playerid)
{
    new Weapon0;
    new Weapon1;
    new Weapon2;
    new Weapon3;
    new Weapon4;
    new Weapon5;
    new Weapon6;
    new Weapon7;
    new Weapon8;
    new Weapon9;
    new Weapon10;
    new Weapon11;
    new Weapon12;
    new Path[25];
    format(Path, sizeof(Path), "\scriptfiles\Accounts\%s.ini", pName(playerid));
    Weapon0 = dini_Int(Path, Weapon0);
    Weapon1 = dini_Int(Path, Weapon1);
    Weapon2 = dini_Int(Path, Weapon2);
    Weapon3 = dini_Int(Path, Weapon3);
    Weapon4 = dini_Int(Path, Weapon4);
    Weapon5 = dini_Int(Path, Weapon5);
    Weapon6 = dini_Int(Path, Weapon6);
    Weapon7 = dini_Int(Path, Weapon7);
    Weapon8 = dini_Int(Path, Weapon8);
    Weapon9 = dini_Int(Path, Weapon9);
    Weapon10 = dini_Int(Path, Weapon10);
    Weapon11 = dini_Int(Path, Weapon11);
    Weapon12 = dini_Int(Path, Weapon12);
    GivePlayerWeapon(playerid, Weapon0, 5000000000);
    GivePlayerWeapon(playerid, Weapon1, 5000000000);
    GivePlayerWeapon(playerid, Weapon2, 5000000000);
    GivePlayerWeapon(playerid, Weapon3, 5000000000);
    GivePlayerWeapon(playerid, Weapon4, 5000000000);
    GivePlayerWeapon(playerid, Weapon5, 5000000000);
    GivePlayerWeapon(playerid, Weapon6, 5000000000);
    GivePlayerWeapon(playerid, Weapon7, 5000000000);
    GivePlayerWeapon(playerid, Weapon8, 5000000000);
    GivePlayerWeapon(playerid, Weapon9, 5000000000);
    GivePlayerWeapon(playerid, Weapon10, 5000000000);
    GivePlayerWeapon(playerid, Weapon11, 5000000000);
    GivePlayerWeapon(playerid, Weapon12, 5000000000);
    return 1;
}

This code is very long, there's a shorter and more effective way to do it:

pawn Code:

public OnPlayerConnect(playerid)
{
    new Weapon[13] /* One more than 12 since 0 wont be used */, Path[25];
    format(Path, sizeof(Path), "\scriptfiles\Accounts\%s.ini", pName(playerid));
    for(new i = 1; i < 13; i++) // Loop - As long as i is less than 13 it repeats the code
    {
        Weapon[i] = dini_Int(path, Weapon[i]); // i is counting +1, every time the code gets called
        GivePlayerWeapon(playerid, Weapon[i], 5000000000); // Give the weapons
    }
    return 1;
}

Using the same code on multiple places

This is the same piece of code, used in multiple callbacks:

pawn Code:

public OnPlayerSpawn(playerid)
{
    if(IsPlayerAdmin(playerid))
    {
        SetPlayerPos(playerid, 1509, -5014, 16);
        SetPlayerInterior(playerid, 3);
        SetPlayerVirtualWorld(playerid, 1);
        SendClientMessage(playerid, -1, "Welcome to the Admin Base");
    }
    return ;
}

CMD:base(playerid, params[])
{
    if(IsPlayerAdmin(playerid))
    {
        SetPlayerPos(playerid, 1509, -5014, 16);
        SetPlayerInterior(playerid, 3);
        SetPlayerVirtualWorld(playerid, 1);
        SendClientMessage(playerid, -1, "Welcome to the Admin Base");
    }
    else return SendClientMessage(playerid, -1, "This command is for RCON admins only");
    return 1;
}

Good example, here we use a stock, which makes it easier to use the code on multiple places, and to edit it:

pawn Code:

public OnPlayerSpawn(playerid)
{
    if(IsPlayerAdmin(playerid)) PutPlayerInBase(playerid);
    return 1;
}

CMD:base(playerid, params[])
{
    if(IsPlayerAdmin(playerid)) PutPlayerInBase(playerid);
    else return SendClientMessage(playerid, -1, "This command is for RCON admins only");
    return 1;
}

stock PutPlayerInBase(playerid) // This makes it easier to edit the script, instead of editing the same thing a few times, you only have to change it once.
{
    SetPlayerPos(playerid, 1509, -5014, 16);
    SetPlayerInterior(playerid, 3);
    SetPlayerVirtualWorld(playerid, 1);
    return SendClientMessage(playerid, -1, "Welcome to the Admin Base");
}

Not using sscanf in an if statement

It's better to use sscanf in an if statement, so that you can give an error when the input isn't valid:

pawn Code:

CMD:kick(playerid, params[])
{
    if(IsPlayerAdmin(playerid))
    {
        new id, reason[75], string[150];
        sscanf(params, "us[75]", id, reason) // Instead of doing this, use an if statement to make sure if the input is valid, so you can send an error if it's not.
        format(string, sizeof(string), "You've been kicked for %s", reason);
        SendClientMessage(id, -1, string);
        Kick(id);
        return 1;
    }
    else return SendClientMessage(playerid, -1, "This command is for RCON admins only");
}

When using sscanf in an if statement:

pawn Code:

CMD:kick(playerid, params[])
{
    if(IsPlayerAdmin(playerid))
    {
        new id, reason[75], string[150];
        if(sscanf(params, "us[75]", id, reason)) return SendClientMessage(playerid, -1, "Syntax: /kick (playerid/name) (reason)");
        format(string, sizeof(string), "You've been kicked for %s", reason);
        SendClientMessage(id, -1, string);
        Kick(id);
        return 1;
    }
    else return SendClientMessage(playerid, -1, "This command is for RCON admins only");
}

Not using switch statements:

This is how most people do it:

pawn Code:

public OnDialogResponse(playerid, dialogid, response, listitem, inputtext[])
{
    if(dialogid == 1) // if statement
    {
        SendClientMessage(playerid, -1, inputtext);
    }
    if(dialogid == 2)  // if statement
    {
        if(listitem == 0) // if statement
        {
            SendClientMessage(playerid, -1, "Listitem 0");
        }
        if(listitem == 1)  // if statement
        {
            SendClientMessage(playerid, -1, "Listitem 1");
        }
    }
    return 1;
}

When using a switch statement:

pawn Code:

public OnDialogResponse(playerid, dialogid, response, listitem, inputtext[])
{
    switch(dialogid) // switch statement
    {
        case 1: SendClientMessage(playerid, -1, inputtext);
        case 2:
        {
            switch(listitem) // switch statement
            {
                case 0: SendClientMessage(playerid, -1, "Listitem 0");
                case 1: SendClientMessage(playerid, -1, "Listitem 1");
            }
        }
    }
    return 1;
}

This is more effective, and easier to use in some cases, another example:

pawn Code:

switch(integer)
{
    case 0: SendClientMessage(playerid, -1, "Case 0");
    case 1: SendClientMessage(playerid, -1, "Case 1");
    case 2..7: SendClientMessage(playerid, -1, "case 2, 3, 4, 5, 6 and 7");
    default: SendClientMessage(playerid, -1, "All Other cases");
}

Not necessary brackets

pawn Code:

public OnPlayerDeath(playerid, killerid, reason)
{
    if(killerid == INVALID_PLAYER_ID)
    {
        SendClientMessageToAll(-1, "This time, the player died because of suicide"); // Since we're only using one line of code, you don't need to use

brackets.
    }
    return 1;
}

Without unnecessary brackets:

pawn Code:

public OnPlayerDeath(playerid, killerid, reason)
{
    if(killerid == INVALID_PLAYER_ID) SendClientMessageToAll(-1, "This time, the player died because of suicide"); // Since we're only using one line of code, you don't need to use brackets.
    return 1;
}

You can use brackets if you want to, but when you have a big script with many useless brackets, it takes longer to scroll trough the code.

Bad indentation:

Always keep your indentation nice, this makes it way easier to see if you're missing a bracket, and it prevents bad indentation warnings:

Bad indentation:

pawn Code:

public OnPlayerConnect(playerid)
{
if(IsPlayerAdmin(playerid)) {
    SendClientMessageToAll(-1, "An Admin Connected, press F12!");
    }
        for(new i = 0; i < MAX_PLAYERS; i++)
{
    new string[20];
        format(string, sizeof(string), "Admins In-Game: ~r~%d", GetOnlineAdmins());
            TextDrawSetString(AdminCount[i], string);
}
return 1;
}

Good indentation:

pawn Code:

public OnPlayerConnect(playerid)
{
    if(IsPlayerAdmin(playerid))
    {
        SendClientMessageToAll(-1, "An Admin Connected, press F12!");
    }
    for(new i = 0; i < MAX_PLAYERS; i++)
    {
        new string[20];
        format(string, sizeof(string), "Admins In-Game: ~r~%d", GetOnlineAdmins());
        TextDrawSetString(AdminCount[i], string);
    }
    return 1;
}

I hope that this tutorial was useful, and that new scripters will understand that some things can be better.
If anyone knows another bad scripting habit, just post it in this topic, and I'll add it when I see it.
Nothing in the code has been tested, so it can contain errors, but the codes are only examples to illustrate my points.

~Yoshi

Re: Bad scripting habits - What not to do in a script - alpha500delta - 09.04.2011

Nice.

Re: Bad scripting habits - What not to do in a script - Calgon - 09.04.2011

You do realize that for loops execute slower than manual loops, right? So part of your tutorial is wrong, manually inputting your code like in your example instead of using a for loop to do it for you is actually worse for speed, the only thing it's good for is cutting the size of your PWN file.

Re: Bad scripting habits - What not to do in a script - Kwarde - 09.04.2011

It looks good, but there is a fail:

Quote:

\scriptfiles\Accounts\%s.ini

You did do new Path[25];. The length of the string is 28. That is one fail. + Playername length, which is 24. So it should be 28+MAX_PLAYER_NAME, which equals to 28+24, which equals to 52. So the Path should be 52 instead of 25 (lol). With what you now have, the string would be:
\scriptfiles\Accounts\Kwa (if my name was "Kwarde" in the game).
And about the switches: It could be a good idea, if you tell about the comma too, and the 'default' (some people don't know it doest exists)

pawn Code:

switch(random(50))
{
    case 1..11,15: SendRconCommand("say Something [1]"); //So, 1,2,3,4,5,6,7,8,9,10,11 or 15
    case 16,17,19: SendRconCommand("say Something [2]"); //So, 16,17 and 19
    default: SendRconCommand("say Something [3]"); //When it's none of above
}

But your tutorial is nice though, well explained etc.

Re: Bad scripting habits - What not to do in a script - [03]Garsino - 09.04.2011

Quote:

Originally Posted by Kwarde

It looks good, but there is a fail:

You did do new Path[25];. The length of the string is 28. That is one fail. + Playername length, which is 24. So it should be 28+MAX_PLAYER_NAME, which equals to 28+24, which equals to 52. So the Path should be 52 instead of 25 (lol). With what you now have, the string would be:
\scriptfiles\Accounts\Kwa (if my name was "Kwarde" in the game).

And you don't need the \scriptfiles\ part.

Re: Bad scripting habits - What not to do in a script - Kwarde - 09.04.2011

Oyeah, thanks for reminding me :P - Forgot that (I never use that \scriptfiles\, so I don't think of it).
So actually, you're now gonna need this maps for it:
"{SAMP DIR}/scriptfiles/scriptfiles/Accounts/"

Lol, double fail in a 'bad scripting' topic. THAT is a cool, epic fail.

AW: Bad scripting habits - What not to do in a script - Nero_3D - 09.04.2011

Some other bad habits

Wasting cells
Using fixed numbers in loops
- Use sizeof array
- Or Macros, so you only need to change one number instead of 20
The function PlayerName which creates each call an array with MAX_PLAYER_NAME (24) cells
- New scripter tend to use the function each line (see some dudb codes)
- Just save the name in an array or just use from the beginning GetPlayerName
Big numbers as decimal, use hex code (2139095040 = 0x7F800000)
Using format to often, its only useful if you insert more parameter

pawn Code:

public OnPlayerSpawn(playerid)
{  //Noone needs weapon at OnPlayerConnect :)
    new //declare the array with the text (9 cells long), no need for a function
        string[9 + MAX_PLAYER_NAME] = "Accounts\\";
    //GetPlayerName instead of PlayerName function
    GetPlayerName(playerid, string[9], MAX_PLAYER_NAME); //we insert the name at the 9 cell
    strcat(string, ".ini"); //and add at the end ".ini", strcat is much faster than format
    for(new i, tmp[16]; ; ++i) //infinite loop
    {
        tmp = "Weapon_"; //set tmp as "Weapon_" (7 cells long)
        valstr(tmp[7], i, false); //and add the number at the end
        tmp[0] = dini_Int(string, tmp); //save the weapon in a variable (because dini_Init is slow)
        if(tmp[0] == 0) //break the loop if no more weapon was found
        {
            break;
        }
        GivePlayerWeapon(playerid, tmp[0], 0x7F800000); //give the player the weapon
    }
    return 1;
}

The unnecessary brackets arent a bad habit, everyone has its own indention style

Nice tutorial in the end

Re: Bad scripting habits - What not to do in a script - Calgon - 09.04.2011

Quote:

Use sizeof array

sizeof works as a constant and generally only returns constants, PAWN uses a static memory allocation system for variable/string sizes, so there's no way that it'd handle a dynamic response, so it is merely a constant it returns. It's only going to harm your compile time, it's hardly inefficient.

Correct me if I'm wrong, but I'm pretty sure I'm right about ^.

AW: Re: Bad scripting habits - What not to do in a script - Nero_3D - 09.04.2011

Quote:

Originally Posted by Calg00ne

Correct me if I'm wrong, but I'm pretty sure I'm right about ^.

Yeah, you are 100% right ^^", the same with a macro but its easier to change

Re: Bad scripting habits - What not to do in a script - Niko_boy - 14.06.2011

Quote:

I hope that this tutorial was useful

You Joking ... this tutorial was awesome