2
\$\begingroup\$

I have created a quest system for a sort of game. The users will be able to complete quests by doing simple things and in return earn rewards for completing them.

Handles everything quest related

public class QuestHandler
{
    private readonly Dictionary<int, Quest> _quests;
    private readonly Dictionary<string, int> _questCount;

    public QuestHandler()
    {
        _quests = new Dictionary<int, Quest>();
        _questCount = new Dictionary<string, int>();
    }

    public void Load()
    {
        _quests.Clear();

        using (var dbConnection = Program.Server.DatabaseHandler.Connection)
        {
            dbConnection.SetQuery("SELECT `id`,`type`,`level_num`,`goal_type`,`goal_data`,`action`,`pixel_reward`,`data_bit`,`reward_type`,`timestamp_unlock`,`timestamp_lock` FROM `quests`");

            using (var reader = dbConnection.ExecuteReader())
            {
                while (reader.Read())
                {
                    _quests.Add(reader.GetInt32("id"), new Quest(
                        reader.GetInt32("id"),
                        reader.GetString("type"),
                        reader.GetInt32("level_num"), 
                        (QuestType)reader.GetInt32("goal_type"),
                        reader.GetInt32("goal_data"), 
                        reader.GetString("action"), 
                        reader.GetInt32("pixel_reward"), 
                        reader.GetString("data_bit"), 
                        reader.GetInt32("reward_type"), 
                        reader.GetInt32("timestamp_unlock"), 
                        reader.GetInt32("timestamp_lock")));

                    AddToCounter(reader.GetString("type"));
                }
            }
        }
    }

    private void AddToCounter(string category)
    {
        if (_questCount.TryGetValue(category, out var count))
        {
            _questCount[category] = count + 1;
        }
        else
        {
            _questCount.Add(category, 1);
        }
    }

    public Quest GetQuestById(int id)
    {
        _quests.TryGetValue(id, out var quest);
        return quest;
    }

    public int GetAmountOfQuestsInCategory(string category)
    {
        _questCount.TryGetValue(category, out var count);
        return count;
    }

    public void ProgressUserQuest(Player session, QuestType questType, int eventData = 0)
    {
        if (session == null || session.GetStats().QuestID <= 0)
        {
            return;
        }

        var userQuest = GetQuestById(session.GetStats().QuestID);

        if (userQuest == null || userQuest.GoalType != questType)
        {
            return;
        }

        var currentProgress = session.GetQuestProgress(userQuest.Id);
        var newProgress = currentProgress;
        var passQuest = false;

        switch (questType)
        {
            default:

                newProgress++;

                if (newProgress >= userQuest.GoalData)
                {
                    passQuest = true;
                }

                break;

            case QuestType.ExploreFindItem:

                if (eventData != userQuest.GoalData)
                {
                    return;
                }

                newProgress = Convert.ToInt32(userQuest.GoalData);
                passQuest = true;
                break;

            case QuestType.StandOn:

                if (eventData != userQuest.GoalData)
                {
                    return;
                }

                newProgress = Convert.ToInt32(userQuest.GoalData);
                passQuest = true;
                break;

            case QuestType.XmasParty:
                newProgress++;
                if (newProgress == userQuest.GoalData)
                {
                    passQuest = true;
                }
                break;

            case QuestType.GiveItem:

                if (eventData != userQuest.GoalData)
                {
                    return;
                }

                newProgress = Convert.ToInt32(userQuest.GoalData);
                passQuest = true;
                break;
        }

        using (var dbConnection = Program.Server.DatabaseHandler.Connection)
        {
            dbConnection.SetQuery("UPDATE `user_quests` SET `progress` = '" + newProgress + "' WHERE `user_id` = '" + session.Id + "' AND `quest_id` = '" + userQuest.Id + "' LIMIT 1");
            dbConnection.ExecuteNonQuery();

            if (passQuest)
            {
                dbConnection.SetQuery("UPDATE `user_stats` SET `quest_id` = '0' WHERE `id` = '" + session.Id + "' LIMIT 1");
                dbConnection.ExecuteNonQuery();
            }
        }

        session.Quests[session.GetStats().QuestID] = newProgress;
        session.SendPacket(new QuestStartedComposer(session, userQuest));

        if (!passQuest)
        {
            return;
        }

        session.GetMessenger().BroadcastAchievement(session.Id, Users.Messenger.MessengerEventTypes.QUEST_COMPLETED, userQuest.Category + "." + userQuest.Name);

        session.GetStats().QuestID = 0;
        session.QuestLastCompleted = userQuest.Id;
        session.SendPacket(new QuestCompletedComposer(session, userQuest));
        session.Duckets += userQuest.Reward;
        session.SendPacket(new HabboActivityPointNotificationComposer(session.Duckets, userQuest.Reward));
        GetList(session, null);
    }

    public Quest GetNextQuestInSeries(string category, int number)
    {
        return _quests.Values.FirstOrDefault(quest => quest.Category == category && quest.Number == number);
    }

    public void GetList(Player session, ClientPacket message)
    {
        var userQuestGoals = new Dictionary<string, int>();
        var userQuests = new Dictionary<string, Quest>();

        foreach (var quest in _quests.Values.ToList())
        {
            if (quest.Category.Contains("xmas2012"))
            {
                continue;
            }

            if (!userQuestGoals.ContainsKey(quest.Category))
            {
                userQuestGoals.Add(quest.Category, 1);
                userQuests.Add(quest.Category, null);
            }

            if (quest.Number < userQuestGoals[quest.Category])
            {
                continue;
            }

            var userProgress = session.GetQuestProgress(quest.Id);

            if (session.GetStats().QuestID != quest.Id && userProgress >= quest.GoalData)
            {
                userQuestGoals[quest.Category] = quest.Number + 1;
            }
        }

        foreach (var quest in _quests.Values.ToList())
        {
            foreach (var goal in userQuestGoals)
            {
                if (quest.Category.Contains("xmas2012"))
                {
                    continue;
                }

                if (quest.Category != goal.Key || quest.Number != goal.Value)
                {
                    continue;
                }

                userQuests[goal.Key] = quest;
                break;
            }
        }

        session.SendPacket(new QuestListComposer(session, message != null, userQuests));
    }

    public void QuestReminder(Player session, int questId)
    {
        var quest = GetQuestById(questId);

        if (quest == null)
        {
            return;
        }

        session.SendPacket(new QuestStartedComposer(session, quest));
    }
}

Holds an instance of each quest...

public class Quest
{
    public readonly string Category;
    public readonly string DataBit;
    public readonly int GoalData;
    public readonly QuestType GoalType;
    public readonly bool HasEnded;
    public readonly int Id;
    public readonly string Name;
    public readonly int Number;
    public readonly int Reward;
    public readonly int RewardType;
    public readonly int TimeUnlock;

    public Quest(int id, string category, int number, QuestType goalType, int goalData, string name, int reward, string dataBit, int rewardType, int timeUnlock, int timeLock)
    {
        Id = id;
        Category = category;
        Number = number;
        GoalType = goalType;
        GoalData = goalData;
        Name = name;
        Reward = reward;
        DataBit = dataBit;
        RewardType = rewardType;
        TimeUnlock = timeUnlock;
        HasEnded = timeLock >= PlusEnvironment.GetUnixTimestamp() && timeLock > 0;
    }

    public string ActionName => HabboUtilities.GetQuestTypeString(GoalType);

    public bool IsCompleted(int userProgress)
    {
        return GoalType == QuestType.ExploreFindItem ? userProgress >= 1 : userProgress >= GoalData;
    }
}

Holds an enum of types of quests

public enum QuestType
{
    FurniMove = 0,
    FurniRotate = 1,
    FurniPlace = 2,
    FurniPick = 3,
    FurniSwitch = 4,
    FurniStack = 5,
    FurniDecorationFloor = 6,
    FurniDecorationWall = 7,
    SocialVisit = 8,
    SocialChat = 9,
    SocialFriend = 10,
    SocialRespect = 11,
    SocialDance = 12,
    SocialWave = 13,
    ProfileChangeLook = 14,
    ProfileChangeMotto = 15,
    ProfileBadge = 16,
    ExploreFindItem = 17,
    SummerEnterRoom = 18,
    AddFriends = 19,
    WaveUsers = 20,
    KissUsers = 21,
    WearHat = 22,
    MassDance = 23,
    GiftOthers = 24,
    StandOn = 25,
    GiveItem = 26,
    GiveCoffee = 27,
    WaveReindeer = 28,
    XmasParty = 29
}
\$\endgroup\$
3
\$\begingroup\$

There is a lot to pick through here.

Firstly is the concept of:


1. Separation of concerns.


The simplest example is, the creation of a thing has nothing to do with the using of a thing. a.k.a you don't need to know how the sausage was made. In your code, the sausage is the database.

I know exactly how your data is stored, what format each field is and more dangerously, embedded in your code, how to delete things. embedded sql is always a red flag.

Imagine a

interface QuestRepository {

void Add(Quest quest);
IQueryable<Quest> Quests{ get; }
void Update(Quest quest);
}

If you had that in your constructor you could perform all the quest handling without any implementation detail knowledge, allowing you to (amongst other things) test the handler without relying on a database.


in your ProgressUserQuest method,

a small thing, you have a Player entity called session, that is a confusing standard, why not just call it player?

a bigger issue is the switch statement. firstly there is a lot of duplication. you are rewriting the same return statements repeatedly. in future have a look at switch case fall through,

though in this case I would say switch case is the wrong thing to use. a switch implies a finite state of cases. usually internal states, that is why they are usually used with enums. enums likewise are a finite state, I would caution against using them for anything likely to change. Fine for days of the week, bad for types of entities.

A better example of solving that problem might be something like a chain of responsibility , something like

interface PlayerQuestStatusUpdater {

   void UpdateQuestStatusFor(Player player,QuestType questType, int eventData);
}

So you would have a chain of possible solutions, each one if checks to see if the type is one it can handle, ...then handle it. that way you have support of unlimited quest types, default quest handlers and you never have to go and update a bunch of case statements.


The above kind of thinking would also remove some of the bad practices of magic strings and hardcoded knowledge, like the honestly baffling random "xmas2012" in the middle of a vague method called "GetList".

Next up we have:


2. Singletons.


Firstly, PlusEnvironment.GetUnixTimestamp() should not be in this code, in-fact calling the Create function on the QuestRepository should compose a quest and hand it the timestamp. it should not be the job of the quest to decide when he is created, what if you are trying to test that? how can you hand it a testable time?

Even worse is HabboUtilities.GetQuestTypeString(GoalType) which is a magic function that goes somewhere using a utility and does something.

Your Quest entity should be an entity. a data transfer object, if you have commands to perform on it, then leave that job to classes that use the quest, like say, the handler. Singletons used in this way will only cause you problems.

Lastly we come full circle:

More Separation of Concerns!


just like the above separation of concern,

Your quest handler is extremely aware of the session object. also the general web naming you have is rather confusing and unnecessary. client packets? notification composers? all of this could be easily wrapped in a

interface QuestAchievements {

   void Broadcast(Achievement achievement):
}

The whole session, notification...stuff, that is someone else's job.


In Conclusion


If I ask you what this class does and your answer contains a string of ands,... it is doing too much. In this case I see about 4-5 tasks.

Try looking at the SOLID principles and giving another go and dividing up your units of work.

\$\endgroup\$
  • \$\begingroup\$ Try to put some titles to sections. It was a pretty good post but content was lost in text sea, like tears in rain. \$\endgroup\$ – A Bravo Dev Jan 3 '18 at 22:32
  • \$\begingroup\$ @ABravoDev good idea, donezel washington. \$\endgroup\$ – apieceoffruit Jan 4 '18 at 0:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.