HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Classes to manage perks to be added and removed

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
perksremovedaddedmanageclassesand

Problem

I am looking for code correctness and design usage as I think I might be over doing it in the class department.

There are two things I'm mostly concerned with.

-
The possible redundancy of classes derived from IPerks. As
CommonPerks, UncommonPerks, etc, all look identical except for
the rows/columns in its internal perk list. I'm thinking I should
just have a concrete PerksList class instead.

-
The use of an enum to keep track of constant values for the
different indices associated with the perkList. All perkList's
will follow the same order, so the indices will always mean the same
thing regardless of which IPerks derived class is being used.

PerkSlotEnums.h

enum class PERK_SLOTS
{
    COMMON_SLOT_COLUMNS = 1,
    COMMON_SCOPE_PERK_COUNT = 2,
    SCOPE_SLOT_INDEX =0,
    STATS_UPGRADE_A_SLOT_INDEX = 1,
    UNIQUE_PERK_A_SLOT_INDEX = 2,
    STATS_UPGRADE_B_SLOT_INDEX = 3,
    UNIQUE_PERK_B_SLOT_INDEX = 4,
    STATS_UPGRADE_C_SLOT_INDEX = 5,
    UNIQUE_PERK_C_SLOT_INDEX = 6
};


IPerks.h

#pragma once
#include "IPerk.h"
#include "IComponent.h"
#include 

class IPerks : public IComponent
{

public:
    IPerks(void);
    virtual ~IPerks(void);
    virtual void AddPerk(IPerk* perk, int column, int row) = 0;
    virtual void RemovePerk(unsigned int index_1, unsigned int index2) =0;
    virtual IPerk* GetPerk(unsigned int perkId) const = 0;
};


CommonPerks.cpp

```
#include "CommonPerks.h"
#include "PerkSlotEnums.h"

CommonPerks::CommonPerks(void)
{
perkList.resize((int)PERK_SLOTS::COMMON_SLOT_COLUMNS);
perkList[(int)PERK_SLOTS::SCOPE_SLOT_INDEX].resize((int)PERK_SLOTS::COMMON_SCOPE_PERK_COUNT);
}

CommonPerks::~CommonPerks(void)
{
}

void CommonPerks::AddPerk(IPerk* perk, int column, int row)
{
perkList[column][row] = perk;
}

void CommonPerks::RemovePerk(unsigned int index_1, unsigned int index_2)
{
if(index_1 < perkList.size())
{
if(index_2 < perkList.at(index_1).size())
{
p

Solution

It is hard to understand what this code does without context.

Strange code

I am looking at this enum and I am asking myself - why?
Is there any way to divide this enum into maybe 3 different enums? That's really strange that you use the same numbers for different elements.

enum class PERK_SLOTS
{
    COMMON_SLOT_COLUMNS = 1,
    COMMON_SCOPE_PERK_COUNT = 2,
    SCOPE_SLOT_INDEX =0,
    STATS_UPGRADE_A_SLOT_INDEX = 1,
    UNIQUE_PERK_A_SLOT_INDEX = 2,
    STATS_UPGRADE_B_SLOT_INDEX = 3,
    UNIQUE_PERK_B_SLOT_INDEX = 4,
    STATS_UPGRADE_C_SLOT_INDEX = 5,
    UNIQUE_PERK_C_SLOT_INDEX = 6
};


C++ is not C

Next, you have:

CommonPerks::CommonPerks(void)


Remove void, it is totally unneccesary. It should be simply

CommonPerks::CommonPerks()


Conventions

Your methods starts with capital letters, that mostly not popular among C++ developers.

virtual void AddPerk(IPerk* perk, int column, int row) = 0;


I suspect that IComponent has a virtual destructor. You do not need to do this.

virtual ~IPerks(void);


Bad naming

Your parameters are badly named, give some more meaningful names.

void CommonPerks::RemovePerk(unsigned int index_1, unsigned int index_2)


Pragma

Pragma is compiler dependent. I would suggest to use headers guards.

Code Snippets

enum class PERK_SLOTS
{
    COMMON_SLOT_COLUMNS = 1,
    COMMON_SCOPE_PERK_COUNT = 2,
    SCOPE_SLOT_INDEX =0,
    STATS_UPGRADE_A_SLOT_INDEX = 1,
    UNIQUE_PERK_A_SLOT_INDEX = 2,
    STATS_UPGRADE_B_SLOT_INDEX = 3,
    UNIQUE_PERK_B_SLOT_INDEX = 4,
    STATS_UPGRADE_C_SLOT_INDEX = 5,
    UNIQUE_PERK_C_SLOT_INDEX = 6
};
CommonPerks::CommonPerks(void)
CommonPerks::CommonPerks()
virtual void AddPerk(IPerk* perk, int column, int row) = 0;
virtual ~IPerks(void);

Context

StackExchange Code Review Q#77765, answer score: 2

Revisions (0)

No revisions yet.