6
\$\begingroup\$

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<AttackSkillData> strikeSkills = new List<AttackSkillData>();
    private List<AttackSkillData> magicSkills = new List<AttackSkillData>();
    private List<AttackSkillData> techSkills = new List<AttackSkillData>();
    private List<AttackSkillData> dualTechSkills = new List<AttackSkillData>();
    private List<AttackSkillData> triTechSkills = new List<AttackSkillData>();

    public SkillsComponent(List<AttackSkillData> strikeSkills, List<AttackSkillData> magicSkills, List<AttackSkillData> techSkills, List<AttackSkillData> dualTechSkills, List<AttackSkillData> 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<AttackSkillData> GetListOfLearnedSkills(SkillAttackType skillAttackType)
    {
        List<AttackSkillData> listToUse = null;
        switch(skillAttackType)
        {
            case SkillAttackType.Strike: { listToUse = strikeSkills; } break;
            case SkillAttackType.Magic: { listToUse = magicSkills; } break;
            case SkillAttackType.Tech: { listToUse = techSkills; } break;
            case SkillAttackType.DualTech: { listToUse = dualTechSkills; } break;
            case SkillAttackType.TriTech: { listToUse = triTechSkills; } break;
        }
        return listToUse;
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Name, TimeEchoes. Combat system mentions "techs" and "magic", where techs double and triple. You sir, are my friend. Apart from that, I'm not sure I'd keep skills in a class that just holds several collections. I'd be inclined to use a Dictionary<SkillAttackType, List<AttackSkillData>>. \$\endgroup\$ – Magus Oct 13 '14 at 19:24
  • \$\begingroup\$ Aha...it didn't even dawn on me to use a Dictionary in that manner, how silly. \$\endgroup\$ – TimeEchoes Oct 13 '14 at 19:33
4
\$\begingroup\$

At this stage I would probably leave the code as is in that I wouldn't consider a more OOP solution. The fact that you have thought about it is good because it means at least you know this possibility exists and it's in the back of your mind in future development. Now, if you find yourself doing another switch statement on SkillAttackType you might want to consider refactoring the code to a more OOP approach. But until then, I like your KISS approach.

In saying that I do have a couple of comments up for discussion.

  1. I always try and keep my variable naming in convention, in both the format of the variable i.e. camelCase, underscore or no, as well as the ordering of the naming. I noticed you have both AttackSkillData and SkillAttackType. IMO it would be better to pick a consistent naming and either run with (SkillAttackType , SkillAttackData) or (AttackSkillType, AttackSkillData).

  2. Even though you are not using OOP you can still minimize the two switches into just one but refactoring it out into a method.

    public void AddNewSkill(SkillAttackType skillAttackType, AttackSkillData data)
    {
      var skillDataList = GetSkillData(skillAttackType);
      skillDataList.Add(data);
    }
    
    public List<AttackSkillData> GetListOfLearnedSkills(SkillAttackType skillAttackType)
    {
        return GetSkillData(skillAttackType);
    }
    
    private void GetSkillData(SkillAttackType skillAttackType)
    {
       // One method used for both public operations
      switch (skillAttackType)
      {
        case SkillAttackType.Strike: return strikeSkills;
        case SkillAttackType.Magic: return magicSkills;
        case SkillAttackType.Tech: return techSkills;
        case SkillAttackType.DualTech: return dualTechSkills;
        case SkillAttackType.TriTech: retirm triTechSkills;
        default:
            throw new NotSupportedException("The following skill " + skillAttackType + " is not supported");
      }
    }
    
  3. Alternatively you could get rid of the multiple private fields and just use a dictionary to maintain your skill data sets. Hence a refactored solution using that method might be.

    public class SkillsComponent
    {
      private readonly Dictionary<AttackSkillType, List<AttackSkillData>> _skillData;
    
      public SkillsComponent(
        List<AttackSkillData> strikeSkills, 
        List<AttackSkillData> magicSkills, 
        List<AttackSkillData> techSkills, 
        List<AttackSkillData> dualTechSkills, 
        List<AttackSkillData> triTechSkills)
      {
          _skillData = new Dictionary<AttackSkillType, List<AttackSkillData>>
          {
             { SkillAttackType.Strike, List<AttackSkillData> strikeSkills },
             { SkillAttackType.Magic, List<AttackSkillData> magicSkills },
             { SkillAttackType.Tech, List<AttackSkillData> techSkills },
             { SkillAttackType.DualTech, List<AttackSkillData> dualTechSkills },
             { SkillAttackType.TriTech, List<AttackSkillData> triTechSkills },
          };
       }
    
        public void AddNewSkill(AttackSkillType skillAttackType, AttackSkillData data)
        {
           var skillDataList = GetSkillData(skillAttackType);
           skillDataList.Add(data);
        }
    
        public List<AttackSkillData> GetListOfLearnedSkills(AttackSkillType skillAttackType)
        {
            return GetSkillData(attackSkillType);
        }
    
        private void GetSkillData(AttackSkillType attackSkillType)
        {
           if(!_skillData.ContainsKey(attackSkillType)
            {
            throw new NotSupportedException("The following skill " + attackSkillType + " is not supported");
            }
    
            return _skillData[attackSkillType); 
        }   
    }
    
\$\endgroup\$
  • \$\begingroup\$ An amazing amount of answers in such a short time. I like Magus's comment and Dreza's explanation for the use of the Dictionary. Your comment on the variable names has been well received! I'm a stickler for good names as well and felt the ones I had were less than ideal, but I couldn't think of a change off the top of my head. \$\endgroup\$ – TimeEchoes Oct 13 '14 at 19:45
  • \$\begingroup\$ Yeah, naming is one of the great joys/trials :) \$\endgroup\$ – dreza Oct 13 '14 at 19:48
4
\$\begingroup\$

The simplest method is to use the Dictionary<T, U> class.

var skills = new Dictionary<SkillAttackType, List<AttackSkillData>>();

You can then simply do:

skills[SkillAttackType.Strike] = new List<AttackSkillData>();
skills[SkillAttackType.Strike].Add( some arbitrary skill );

Keeps it simple and concise.

I'm rather interested in what you end up with, as a big fan of what this must be based on.

\$\endgroup\$
4
\$\begingroup\$

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<AttackSkill>() instead of five different lists.

Your code should then look like this;

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();
    }
}

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

List<AttackSkill> 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!

\$\endgroup\$
  • \$\begingroup\$ I'm going to keep this one in the quiver, I can see uses for it elsewhere in my code that don't exactly use enumerations. Thank you! \$\endgroup\$ – TimeEchoes Oct 13 '14 at 19:47
  • \$\begingroup\$ Relooking at this, it won't quite work as you would need to do a .Select(p => p.Data) on your Where clause. So I would consider first doing a skills.SingleOrDefault(). Then check for null condition (just in case a new skill is added that we don't know about. Then return the .Data of that?? \$\endgroup\$ – dreza Oct 13 '14 at 19:52
  • \$\begingroup\$ @dreza why wouldn't this work? Where() returns an IEnumerable<AttackSkill> in this case, which can be parsed to a List<AttackSkill>(). Heck, if you would want to you could immediately loop over it using foreach(var attackSkill in skills.Where(s => s.type == SkillAttackType.Strike). \$\endgroup\$ – Dion V. Oct 13 '14 at 20:48
  • \$\begingroup\$ @TimeEchoes you're welcome! :) \$\endgroup\$ – Dion V. Oct 13 '14 at 20:52
  • \$\begingroup\$ @DionV. I would have thought it wouldn't work as is because your Linq query is returning a List<AttackSkill> where your method definition states it is to return a List<AttackSkillData>. Probably just a typo though. \$\endgroup\$ – dreza Oct 13 '14 at 20:53

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.