Help with Creating Functions for cleaner code.

I’m looking to clean up my current script (It works with some issues! Yay for progress) Right now what I have is

void OnLogin(Player* player, bool firstLogin)
{
uint32 rewardFrequency, TEAM_ALLIANCE, TEAM_HORDE,ALLIANCE_MOUNT, HORDE_MOUNT, playedtime, lastrewarded, newplaytime, MinutesToReward, ItemReward, InitialReward, StandardReward, MOUNTONSTART;
MOUNTONSTART = 1;
MinutesToReward = 5;
ItemReward = 29434;
InitialReward = 10;
StandardReward = 1;
rewardFrequency = MinutesToReward*60;
TEAM_ALLIANCE = 469;
TEAM_HORDE = 67;
ALLIANCE_MOUNT = 66090;
HORDE_MOUNT = 64659;

   auto guid = player->GetGUID();
   if(firstLogin) {

       if(MOUNTONSTART == 1) {
           if (player->GetTeam() == TEAM_ALLIANCE) {
               player->LearnSpell(ALLIANCE_MOUNT, false);
           }
           if (player->GetTeam() == TEAM_HORDE) {
               player->LearnSpell(HORDE_MOUNT, false);
           }
       }
   }
   QueryResult lastreward = CharacterDatabase.PQuery("SELECT last_reward FROM player_rewards WHERE guid = %u",guid);
   QueryResult totaltime = CharacterDatabase.PQuery("SELECT totaltime FROM characters WHERE guid = %u", guid);
   Field *played = totaltime->Fetch();
   playedtime = played[0].GetUInt32();

if(!lastreward) {
ChatHandler(player->GetSession()).PSendSysMessage(“You have received 10 loyalty points”);
CharacterDatabase.PQuery(“INSERT INTO player_rewards(guid,last_reward) VALUES(%u, %u)”, guid, playedtime);
player->AddItem(ItemReward, InitialReward);

} else {
Field rewarded = lastreward->Fetch();
lastrewarded = rewarded[0].GetUInt32();
if(playedtime >= lastrewarded + rewardFrequency) {
uint amounttogrant, timecheck, timecheck2;
timecheck = lastrewarded + rewardFrequency;
timecheck2 = playedtime - timecheck;
amounttogrant = timecheck2/rewardFrequency+1;
ChatHandler(player->GetSession()).PSendSysMessage(“Grant %u Points”, amounttogrant);
//Update Script
newplaytime = amounttogrant
60 + lastrewarded;
CharacterDatabase.PQuery(“UPDATE player_rewards SET last_reward = %u WHERE guid = %u”, playedtime, guid);
if(amounttogrant == 1) {
ChatHandler(player->GetSession()).PSendSysMessage(“%u Loyalty Point Has been added to your currency.”, amounttogrant);
} else {
ChatHandler(player->GetSession()).PSendSysMessage(“%u Loyalty Points Have been added to your currency.”, amounttogrant);
}
player->AddItem(ItemReward, amounttogrant*StandardReward);
}
uint32 timecheck = lastrewarded + rewardFrequency;
uint32 lastrewardedMinutes = lastrewarded / 60;
uint32 rewardFrequencyMinutes = rewardFrequency /60;
uint32 timecheckMinutes = timecheck / 60;
uint32 playedtimeMinutes = playedtime /60;
uint32 nextreward = timecheckMinutes - playedtimeMinutes;
if(nextreward == 1) {
ChatHandler(player->GetSession()).PSendSysMessage(“Next Loyalty Reward in %u minute.”,nextreward);
} else {
ChatHandler(player->GetSession()).PSendSysMessage(“Next Loyalty Bonus in %u minutes”, nextreward);
}
}
}

What I'd like to do is clean this up. A lot I mean...

For instance

void OnLogin(Player* player, bool firstLogin)
{
uint32 rewardFrequency, TEAM_ALLIANCE, TEAM_HORDE,ALLIANCE_MOUNT, HORDE_MOUNT, playedtime, lastrewarded, newplaytime, MinutesToReward, ItemReward, InitialReward, StandardReward, MOUNTONSTART;
MOUNTONSTART = 1;
MinutesToReward = 5;
ItemReward = 29434;
InitialReward = 10;
StandardReward = 1;
rewardFrequency = MinutesToReward*60;
TEAM_ALLIANCE = 469;
TEAM_HORDE = 67;
ALLIANCE_MOUNT = 66090;
HORDE_MOUNT = 64659;

   auto guid = player->GetGUID();
   if(firstLogin) { 
   RewardNewPlayer(Player* player, guid, ALLIANCE_MOUNT, HORDE_MOUNT, MOUNTONSTART);
   } 

}
void RewardNewPlayer(Player* player, uint32 guid, uint32 ALLIANCE_MOUNT, uint32 HORDE_MOUNT, uint32 MOUNTONSTART) {
ChatHandler(player->GetSession()).PSendSysMessage(“The Function Was Called”);
}

Of course the function would be to reward the new player. Not tell them the function was called, but that's just an 
Of course the function would be to reward the player not to tell them we called a function, but that's just an example of what I'm trying to do. I believe my issue lies in Player* player. My PHP Example of what I'm trying to learn how to do in C++ would be.

function OnLogin($player = Player, bool $firstLogin) {
//Pretend All the variables are set here…
//Then we would call it as
RewardNewPlayer(Player, $guid, $ALLIANCE_MOUNT, $HORDE_MOUNT, $MOUNTONSTART);
}
function RewardNewPlayer($player = null, $guid, $ALLIANCE_MOUNT, $HORDE_MOUNT, $MOUNTONSTART) {

}

RewardNewPlayer(Player* player, guid, ALLIANCE_MOUNT, HORDE_MOUNT, MOUNTONSTART);

to

RewardNewPlayer(player, guid, ALLIANCE_MOUNT, HORDE_MOUNT, MOUNTONSTART);

There seem to be some issues with your code indeed.
You should check if lastreward exists before using it. Same for the other query results.
you should use the counter, not the guid directly in the DB queries because the guid and %u dont mix.

So I looked through the code. Redid the code. And I think it’s a lot better at this point. The only thing is I have no clue what you mean by using the counter, and I just finished with this.

#include “ScriptMgr.h”
#include “Player.h”
#include “Chat.h”
#include “World.h”
#include
#include
#include

#define MOUNT_ENABLED 1
#define HORDE_MOUNT 55531 //Mechano Hog
#define ALLIANCE_MOUNT 60424 //Mechageer
#define INITIAL_REWARD 10 //How many are granted on first login
#define STANDARD_REWARD 1 //How many are granted each interval
#define REWARD_ITEM 29434 //Badge of Justice
#define MINUTES_TO_AWARD 30 //How many minutes it takes to get the reward
//Don’t Change After this unless you know what you’re doing
#define MINUTES_AWARD MINUTES_TO_AWARD * 60
using namespace std;

int randomNumber(int max)
{
int random_number;
//max = 100;
srand(time(0));
random_number = (rand () % max) + 1;
return random_number;
}
int GenerateMount(int generator)
{
//Maybe Throw this into the works with the login and randomnumber generator?
int MECHANO_BIKE, GRAND_ICE_MAMMOTH, QIRAJI_LAND, SWIFT_SPECTRAL_TIGER;
int CRIMSON_DEATHCHARGER, WHITE_HAWK, ZULIAN, BASE_MOUNT;
if(generator == 1) {
//Mechano Hog 1%
}
if(generator >=2 && generator <=10) {
//SWIFT_SPECTRAL 9%
}
if(generator >=11 && generator <=20) {
//Black Qiraji Battle Tank 9%
}
if(generator >= 21 && generator <=30) {
//Crimson DeathCharger 9%
}
if(generator >=31 && generator <=40) {
//Swift White HawkStrider 9%
}
if(generator >=41 && generator <= 45) {
//4% Chance to Have chance at Grand Ice Mammoth
int RAND = randomNumber(1000);
if(RAND >= 1 && RAND <=10) {
//Ice Mammoth .10%
}
else {
//Base Mount 99.90%
}
}
if(generator >=46 && generator <= 100) {
//Base Mount 59%
}
}

class PlayerRewards : public PlayerScript
{
public:
PlayerRewards() : PlayerScript(“PlayerRewards”) { }

int GetPlayTime(uint32 guid) {
    uint32 PlayedTime;
    QueryResult PlayTime = CharacterDatabase.PQuery("SELECT totaltime FROM characters WHERE guid = %u", guid);
    Field *Played = PlayTime->Fetch();
    PlayedTime = Played[0].GetUInt32();
    return PlayedTime;
}
int GetLastTime(uint32 guid) {
    uint32 LastRewarded;
    QueryResult LastReward = CharacterDatabase.PQuery("SELECT last_reward FROM player_rewards WHERE guid = %u", guid);
    Field *Rewarded = LastReward->Fetch();
    LastRewarded = Rewarded[0].GetUInt32();
    return LastRewarded;
}
void CheckLastReward(Player * player, uint32 guid) {
    QueryResult LastReward = CharacterDatabase.PQuery("SELECT last_reward FROM player_rewards WHERE guid = %u", guid);
    if(!LastReward) {
        CreateRewardData(player, guid);
    } else {
        CheckTimer(player, guid);
    }
}
void CheckTimer(Player * player, uint32 guid) {
    uint32 LastRewardMinutes, PlayTimeMinutes, ElapsedTime;
    LastRewardMinutes = GetLastTime(guid) /60;
    PlayTimeMinutes = GetPlayTime(guid) /60;
    ElapsedTime = PlayTimeMinutes - LastRewardMinutes;
    if(ElapsedTime >= MINUTES_TO_AWARD) {
        uint32 Grantable = PlayTimeMinutes - LastRewardMinutes;
        uint32 Granting =  Grantable/MINUTES_TO_AWARD;
        uint32 NewRewardTime = PlayTimeMinutes*60;
        GrantReward(player, guid, 2, Granting);
    }
    else {
        //Do Nothing
    }

}
void NewPlayerWelcome(Player * player, uint32 guid) {
    if(MOUNT_ENABLED == 1) {
        if (player->GetTeam() == 469) {
            player->LearnSpell(ALLIANCE_MOUNT, false);
        }
        if (player->GetTeam() == 67) {
            player->LearnSpell(HORDE_MOUNT, false);
        }
    }
    CreateRewardData(player, guid);
}
void CreateRewardData(Player * player, uint32 guid) {
    uint32 PlayedTime;
    PlayedTime = GetPlayTime(guid);
    CharacterDatabase.PQuery("INSERT INTO player_rewards(guid, last_reward) VALUES(%u, %u)", guid,PlayedTime);
    GrantReward(player, guid, 1, 0);
}
void GrantReward(Player * player, uint32 guid, uint32 type, uint32 INCOMING) {
    switch(type) {
        case 1:
            player->AddItem(REWARD_ITEM, INITIAL_REWARD);
            break;
        case 2:
            uint32 Rewarded = STANDARD_REWARD*INCOMING;
            player->AddItem(REWARD_ITEM, Rewarded);
            UpdateLastReward(guid);
            break;
    }
}
void UpdateLastReward(uint32 guid) {
    uint32 PlayedTime = GetPlayTime(guid);
    CharacterDatabase.PQuery("UPDATE player_rewards SET last_reward = %u WHERE guid = %u", PlayedTime, guid);
}

void OnLogin(Player* player, bool firstLogin) {
auto guid = player->GetGUID();
uint32 RAND, rewardFrequency, playedtime, lastrewarded;
uint32 newplaytime, MinutesToReward, ItemReward, InitialReward, StandardReward;
MinutesToReward = 1;
rewardFrequency = MinutesToReward*60;
if(firstLogin) {
NewPlayerWelcome(player, guid);
}
else {
uint32 PlayedTime;
PlayedTime = GetPlayTime(guid);
CheckLastReward(player, guid);
}
}
};

void AddSC_PlayerRewards()
{
new PlayerRewards();
}

I don't think I'm fully done with this one, but I feel like some of my ideas are branching to a different custom patch at this rate.

Did some digging and just had to switch

[COLOR=rgb(0,0,128)]auto guid = player->GetGUID();

to

uint32[COLOR=rgb(0,0,128)] guid = player->GetGUID().GetCounter();

And since everything else was derived off of that for the guid it got it =D Thanks Rochet. Hopefully this one's a little friendlier to look at.

it is very very very bad to execute sync queries in any kind of script

What kind of issues are there, can you elaborate?

Please. As far as I can tell it’s the best way to store information, but if I’m doing something here that’s bad practice I’d like to know while learning so I can throw away bad coding habits now and not after they’re well ingrained.

Database queries fall in the category of I/O, like network, hard disk, etc. All I/O should happen on a background thread in asynchronous mode instead of blocking the current thread, causing performance bottlenecks.

This is why we load all player related data in asynch thread at https://github.com/TrinityCore/TrinityCore/blob/3.3.5/src/server/game/Handlers/CharacterHandler.cpp#L756 and then process the data in the synchronous main thread once ready at https://github.com/TrinityCore/TrinityCore/blob/3.3.5/src/server/game/Server/WorldSession.cpp#L1119 .
https://github.com/TrinityCore/TrinityCore/blob/3.3.5/src/server/game/Handlers/CharacterHandler.cpp#L64 shows the list of all different SQL statement queued together in the asynch thread, you could add yours there with very little impact on performances (saving the data in a Player class member and reading the value in your custom script)

I was trying to avoid any core modification, but I’ll work on that next lol.