2
\$\begingroup\$

Im creating a class that has all of my data in and can see it would quickly become messy.

I feel that there must be a way of making it neater rather than making a new method each time

For example. I have a character that out of the huge skill list each with different values for different things, they just have 1h weapon and shield. I declare this by

Character character = new Character();
character.Skills.Add(Skill.OneHanded());
character.Skills.Add(Skill.Shield());

it also makes it fairly easy if they level up and learn a new skill as i can just do

character.Skills.Add(Skill.Polearm());

however currently my Skill class looks like a mess. (note that there will be many more skills (about 30 in total) and each one will have more attributes such as applicable weapons, level requirements etc, but not currently coded.

public class Skill
{
    public string Description { get; set; }
    public bool Advanced{ get; set; }
    public int PointsToLearn{ get; set; }



    public static Skill OneHanded()
    {
        return new Skill
        {
            Description = "Skill needed for the use of one handed weapons",
            Advanced = false,
            PointsToLearn= 1
        };
    }
    public static Skill TwoHanded()
    {
        return new Skill
        {
            Description = "Skill needed for use of two handed weapons.",
            Advanced= false,
            PointsToLearn = 2
        };
    }
    public static Skill Polearm()
    {
        return new Skill
        {
            Description = "Skill needed for use of polearms",
            Advanced = false,
            PointsToLearn = 3
        };
    }
    public static Skill Shield()
    {
        return new Skill
        {
            Description = "Skill needed to use shields",
            Advanced = false,
            PointsToLearn = 1
        };
    }
    public static Skill ResistMagic()
    {
        return new Skill
        {
            Description = "Negates the first spell cast at the character",
            Advanced = true,
            PointsToLearn = 0
        };
    }
}

As requested the Character class is

class Character
    {
        public List<Skill> Skills = new List<Skill>();
        public int Level { get; set; }
        public string Name { get; set; }
        public int Command { get; set; }
        public int Coordination { get; set; }
        public int Reflex { get; set; }

    }

There are many characters (i guess think fire emblem if you have ever played that) each one can be skilled and levelled (and die) independently . The class is mostly to keep a record of all characters that have joined the players mercenary company, with their current skills, attributes, level etc etc.

It will called when ever a new character joins the company and added to a character list and then remain in the list ready to be called with the attributes of the character when needing to know information about them.

is there a way to simply this? I also got advised I could do

public class Skill
{
    public enum Skills{ OneHanded,TwoHanded,Polearm,Shield};
}

which opens up using Skill.Skills."Skill" but I'm not sure how I would set the data against each of the options.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I'm afraid this question does not match what this site is about. Code Review is about improving existing, working code. The example code that you have posted is not reviewable in this form because it leaves us guessing at your intentions. Unlike Stack Overflow, Code Review needs to look at concrete code in a real context. Please see Why is hypothetical example code off-topic for CR? \$\endgroup\$ – Dannnno Apr 27 '18 at 14:17
  • \$\begingroup\$ i shall tidy up the example to meet guidlines bare with me :) \$\endgroup\$ – Zarwalski Apr 27 '18 at 14:22
  • \$\begingroup\$ Could add more context. What is Character? How is it implemented? \$\endgroup\$ – t3chb0t Apr 27 '18 at 14:37
  • \$\begingroup\$ Hi t3chb0t, tried to expand the context. \$\endgroup\$ – Zarwalski Apr 27 '18 at 14:45
  • \$\begingroup\$ The class is mostly to keep a record of all characters that have joined the players mercenary company does it mean that you have multiple instances of this class in another collection or how are storing each character? \$\endgroup\$ – t3chb0t Apr 27 '18 at 14:52
2
\$\begingroup\$

In the way you show that you create each Skill instance, you really have anything than the Description to distinguish them from each other.

I read your Skill class as merely an advanced flag that defines a small set of properties. In the below I further anticipate, that each skill type doesn't change throughout the game. If that is correct, then I would make the Skill class as a "singleton" per Skill type (onehanded, twohanded etc.) and let a static Factory method create/return the right Skill according to a Skill Type enum:

public class Skill
{
  public string Description { get; private set; }
  public bool Advanced { get; private set; }
  public int PointsToLearn { get; private set; }
  public SkillType Type { get; private set; }

  private Skill()
  {
  }

  private static Dictionary<SkillType, Skill> s_skills = new Dictionary<SkillType, Skill>
  {
    { SkillType.OneHanded, new Skill { Type = SkillType.OneHanded, Description = "Skill needed for the use of one handed weapons", Advanced = false, PointsToLearn = 1, } },
    { SkillType.TwoHanded, new Skill { Type = SkillType.TwoHanded, Description = "Skill needed for use of two handed weapons", Advanced = false, PointsToLearn = 2, } },
    { SkillType.Polearm, new Skill { Type = SkillType.Polearm, Description = "Skill needed for use of polearms", Advanced = false, PointsToLearn = 3, } },
    { SkillType.Shield, new Skill { Type = SkillType.Shield, Description = "Skill needed to use shields", Advanced = false, PointsToLearn = 1, } },
    { SkillType.ResistMagic, new Skill { Type = SkillType.ResistMagic, Description = "Negates the first spell cast at the character", Advanced = true, PointsToLearn = 0, } },
  };

  public static Skill Factory(SkillType type)
  {
    return s_skills[type];
  }
}

public enum SkillType
{
  OneHanded,
  TwoHanded,
  Polearm,
  Shield,
  ResistMagic
}

If their role is more advanced than that with methods and state, it IMO calls for at class hierarchy in some way like:

public abstract class Skill
{
  public string Description { get; set; }
  public bool Advanced { get; set; }
  public int PointsToLearn { get; set; }
  public SkillLevel SkillLevel { get; set; }
}

public class OneHanded: Skill
{

}

public class TwoHanded : Skill
{

}
...

which of cause requires a lot more work.

Update

If you want to filter a list of skills by SkillLevel, it can be done like this:

List<Skill> skills = new List<Skill> { new OneHanded(), new TwoHanded(), ... etc. }

foreach (Skill skill in skills.Where(s => s.SkillLevel == SkillLevel.Basic))
{
  Console.WriteLine(skill.GetType().Name);
}

Beside that I would change the Advanced flag to an enum too, because you can then distinguish between more levels like:

enum SkillLevel
{
  Basic,
  Medium,
  Advanced,
}
\$\endgroup\$
  • \$\begingroup\$ I tried making it into class heirarchy, but now I'm getting stack overflow errors public class Skill { public Onehanded onehanded = new Onehanded(); public string Description { get; set; } public string SkillName { get; set; } public int PointsToLearn { get; set; } public bool Advanced { get; set; } public int Xp; public class Onehanded : Skill { public string Description = "Skill needed for the use of one handed weapons"; } } \$\endgroup\$ – Zarwalski Apr 30 '18 at 8:32
  • \$\begingroup\$ doesn't matter i worked out my mistake :) Thanks again xx \$\endgroup\$ – Zarwalski Apr 30 '18 at 8:48
  • \$\begingroup\$ with the nested classes I now have something that looks like character.Skill.Onehanded.SkillLevel and character.Skill.TwoHanded.SkillLevel etc etc If i wanted to cycle over them such as foreach (something) where SkillLevel = Basic, how would i now select all skills where the SkillLevel value = Basic, there isnt a foreach class in class :( \$\endgroup\$ – Zarwalski Apr 30 '18 at 9:28
  • \$\begingroup\$ @Zarwalski: I'm not sure, I understand what you mean by "nested classes". class OneHanded, class TwoHanded etc. shouldn't be nested classes of Skill, but subclasses of it in a normal OOP manner. But see my update, that shows how to filter a list of skills. \$\endgroup\$ – Henrik Hansen Apr 30 '18 at 10:21
  • \$\begingroup\$ The filter a list of skills by SkillLevel throws OOP out the window. There should be a domain relevant class, and it is not battlearray BTW. A group of soldiers may be termed squad, horde, regiment, host, century, phalanx, etc. An "array" is coding implementation detail which should be abstracted - hidden - by said class. \$\endgroup\$ – radarbob Apr 30 '18 at 19:37
0
\$\begingroup\$

You should override equals and use HashSet so the same skill cannot be entered twice.

Description is generic but I think this makes cleaner code.

public enum SkillType { OneHanded, TwoHanded, Polearm, Shield }
public class Skill
{
    private Dictionary<SkillType, bool> TypeToAdvanced = new Dictionary<SkillType, bool> { { SkillType.OneHanded, true },
                                                                                            { SkillType.Polearm,   true },
                                                                                            { SkillType.Shield,   false },
                                                                                            { SkillType.Shield,    true }, };
    private Dictionary<SkillType, int> TypeToPoints = new Dictionary<SkillType, int> { { SkillType.OneHanded, 1 },
                                                                                        { SkillType.Polearm,   2 },
                                                                                        { SkillType.Shield,    2 },
                                                                                        { SkillType.Shield,    1 }, };
    protected bool Equals(Skill other)
    {
        return this.Type == other.Type;
    }
    public override bool Equals(object obj)
    {
        if (ReferenceEquals(null, obj)) return false;
        if (obj.GetType() != this.GetType()) return false;
        return Equals((Skill)obj);
    }
    public override int GetHashCode()
    {
        return (int)Type;
    }

    public SkillType Type { get; }
    public string Description { get { return $"Skill needed for use of {Type.ToString()}"; } }
    public int PointsToLearn { get { return TypeToPoints[Type]; } }
    public bool Advancedd { get { return TypeToAdvanced[Type]; } }
    public Skill (SkillType type)
    {
        Type = type;
    }
\$\endgroup\$
  • \$\begingroup\$ Great idea, and I've seen it used effectively before, but here it's inappropriate. If a character were defined by a single SkillType then good. But here we're only trying to prevent duplicate skills for a given character. That's been engineered into the .NET Dictionary class already with Dictionary.Contains(). So Character.AddSkill() should simply use that. \$\endgroup\$ – radarbob Apr 30 '18 at 19:54
  • \$\begingroup\$ @radarbob Buy a vowel. OP does not use Dictionary and even it OP did if without override Equals could add a duplicate Skill. There is no Character.AddSkill in the code. \$\endgroup\$ – paparazzo Apr 30 '18 at 20:19
  • \$\begingroup\$ So assuming public List<Skill> Skills. Ok. After much thought about domain design vs mere coding issues I'll drop the the issue. Equals override works, as you said. Next, though, character.Skills.Add violates least knowledge principle. A class, i.e. Character, should hide state and expose functionality. \$\endgroup\$ – radarbob May 1 '18 at 5:46
  • \$\begingroup\$ @radarbob A am happy you gave it much thought and decided to drop the issue. \$\endgroup\$ – paparazzo May 1 '18 at 8:15
  • \$\begingroup\$ I am retracting my statement ".. here it's inappropriate". A certain familiar use case was driving that thought. Now, an observation: Equals override can result in really beautiful code. .NET collections are smart here, it's "in the DNA" of the class and so client code can be clean and simple. One time (not at band camp) a single, simple, short statement compared two collections returning a collection of duplicates. And the underlying code was quite simple too - all the way down to the Equals overrides (turtles, all the way down). But it must be OOP all the way down too. \$\endgroup\$ – radarbob May 1 '18 at 20:13

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.