patterncsharpMinor

Listing skills for role-playing characters

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

skillsplayingrolelistingforcharacters

Problem

After I completed writing this code, I started to wonder if the switches should be replaced with a more OOP approach. I originally picked the switch approach because there will never be anymore skill groups, only the 5 listed. It also made it a bit easier on the GUI side of things to associate a selection index to one of the enum values.

I figure the KISS rule applies since the structure of the data is going to be concrete, but I'm always open to broadening my view.

```
public class SkillsComponent
{
private List strikeSkills = new List();
private List magicSkills = new List();
private List techSkills = new List();
private List dualTechSkills = new List();
private List triTechSkills = new List();

public SkillsComponent(List strikeSkills, List magicSkills, List techSkills, List dualTechSkills, List triTechSkills)
{
this.strikeSkills = strikeSkills;
this.magicSkills = magicSkills;
this.techSkills = techSkills;
this.dualTechSkills = dualTechSkills;
this.triTechSkills = triTechSkills;
}

public void AddNewSkill(SkillAttackType skillAttackType, AttackSkillData data)
{
switch (skillAttackType)
{
case SkillAttackType.Strike: { strikeSkills.Add(data); } break;
case SkillAttackType.Magic: { magicSkills.Add(data); } break;
case SkillAttackType.Tech: { techSkills.Add(data); } break;
case SkillAttackType.DualTech: { dualTechSkills.Add(data); } break;
case SkillAttackType.TriTech: { triTechSkills.Add(data); } break;
}
}

public List GetListOfLearnedSkills(SkillAttackType skillAttackType)
{
List listToUse = null;
switch(skillAttackType)
{
case SkillAttackType.Strike: { listToUse = strikeSkills; } break;
case SkillAttackType.Magic: { listToUse = magicSkills; } break;
case SkillAttackType.Tech: { listToUse = techSkills; } break;
cas

Solution

It might be a bit cleaner if you introduce a new class (or struct) containing both AttackSkillData and SkillAttackType.

Consider something like this;

public class AttackSkill
{
    public AttackSkillData data;
    public SkillAttackType type;
}

And consider adding a List() instead of five different lists.

Your code should then look like this;

public class SkillsComponent
{
    private List skills = new List();

    public SkillsComponent(List listOfAllSkills)
    {
        this.skills = listOfAllSkills;
    }

    public void AddNewSkill(SkillAttackType skillAttackType, AttackSkillData data)
    {           
        skills.Add(new AttackSkill{ type = skillAttackType, data = data});     
    }

    public List GetListOfLearnedSkills(SkillAttackType skillAttackType)
    {
        return skills.Where(s => s.type == skillAttackType).ToList();
    }
}

You can then acces any type of skill using a Where() where needed. For example, to get all skills having SkillAttackType.Strike;

List strikeSkills = skills.Where(s => s.type == SkillAttackType.Strike).ToList();

It will make your code less noisy and you will be rid of the switch. Double score!

Code Snippets

public class AttackSkill
{
    public AttackSkillData data;
    public SkillAttackType type;
}

public class SkillsComponent
{
    private List<AttackSkill> skills = new List<AttackSkill>();

    public SkillsComponent(List<AttackSkill> listOfAllSkills)
    {
        this.skills = listOfAllSkills;
    }

    public void AddNewSkill(SkillAttackType skillAttackType, AttackSkillData data)
    {           
        skills.Add(new AttackSkill{ type = skillAttackType, data = data});     
    }

    public List<AttackSkill> GetListOfLearnedSkills(SkillAttackType skillAttackType)
    {
        return skills.Where(s => s.type == skillAttackType).ToList();
    }
}

List<AttackSkill> strikeSkills = skills.Where(s => s.type == SkillAttackType.Strike).ToList();

Context

StackExchange Code Review Q#66568, answer score: 4

Revisions (0)

No revisions yet.