6
\$\begingroup\$

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.

  1. 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.

  2. 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 <vector> 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()) { perkList.at(index_1).at(index_2) = nullptr; } } } IPerk* CommonPerks::GetPerk(unsigned int index_1, unsigned int index_2) const { if(index_1 < perkList.size()) { if(index_2 < perkList.at(index_1).size()) { return perkList.at(index_1).at(index_2); } } return nullptr; }
\$\endgroup\$
3
  • \$\begingroup\$ Is perkList a 2D vector? Why do you have a column and a row here? Do you not want to just store the "perks" by value in some sort of container? Additionally is CommonPerks a derived class of IPerks? Do you have the newer c++11 available to you? \$\endgroup\$ Commented Jun 2, 2015 at 3:51
  • 5
    \$\begingroup\$ I'd like to post an answer but I'm heavily leaning towards a redesign and it's impossible to really give any good comment on the design without knowing what it is you are tying to do here. So problem is this code is trying to solve? Tell us a bit about the overall design here. \$\endgroup\$ Commented Jun 20, 2015 at 18:20
  • \$\begingroup\$ This question is incomplete. To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. The current title states your concerns about the code; it needs an edit to simply state the task; see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ Commented Apr 9, 2018 at 15:10

1 Answer 1

Reset to default
2
\$\begingroup\$

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.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.