4
\$\begingroup\$

While the experience is limited in C#, it is not in OOP generally. While the Beginner-tag is used, relentless, harsh (but constructive) feedback is still wanted. It is more for you to get a grasp of the experience in C# so far.

Would you want this code in your next AAA game title? If not, what are the problem(s)? That being said, the purpose is not to write perfect code, it should be good, acceptable is not acceptable.

The idea is to have a Character that in turn declares and initializes its fields. For now they are:

  • Level, the character´s current level.
  • ExpCur, how much experience is achieved on this level.
  • ExpTnl, the experience needed to advance a level.
  • Attributes, a Dictionary storing the two existing attributes, Offensive and Defensive (they can be found in AttributeConstants.cs) with the name as the keys.
  • AttributePoints, how many remaining points can be spent on an Attribute.

To those fields there are some helpers, nothing should directly be exposed outside of Character.cs, if a change to an instance Character is needed Character.cs should provide a mutator of some sort for that value.

Character.cs

using System;
using System.Collections.Generic;

namespace Character
{
    class Character
    {
        public int Level { get; private set; }
        public int ExpCur { get; private set; }
        public int ExpTnl { get; private set; }

        private Dictionary<string, Attribute> Attributes = new Dictionary<string, Attribute>();
        public int AttributePoints { get; private set; }

        public Character (int level)
        {
            Level = (level > 0 ) ? level : 1;
            ExpCur = 0;
            ExpTnl = 100 * level;

            foreach (String s in AttributeConstants.AttributeNames)
            {
                Attributes.Add(s, new Attribute (AttributeConstants.DefaultValue));
            }

            AttributePoints = (level - 1) * AttributeConstants.AttributePointsPerLevel;
        }

        public int AddExp (int exp)
        {
            int curLevel = Level;
            ExpCur += exp;

            while (ExpCur >= ExpTnl)
            {
                ExpCur -= ExpTnl;
                ExpTnl = 100 * Level;
                Level++;
                AttributePoints += AttributeConstants.AttributePointsPerLevel;
            }

            return Level - curLevel;
        }

        public AttributeUnmodifiable GetAttribute (string name)
        {
            return new AttributeUnmodifiable(Attributes[name].Value, Attributes[name].Modifiers);
        }

        public void IncAttribute (string name)
        {
            if (AttributePoints > 0)
            {
                AttributePoints--;
                Attributes[name].Inc ();
            }
        }
    }
}

AttributeConstants.cs

This class purpose is to gather all constants for attributes. So that there are no hidden constants all over the place.

namespace Character
{
    public class AttributeConstants
    {
        internal static readonly string[] AttributeNames = {
            "Offensive",
            "Defensive",
        };

        internal static readonly string[] AttributeDescriptions = {
            "Your damage. Lorem Ipsum...",
            "Your defense. Lorem Ipsum...",
        };

        internal static readonly int DefaultValue = 5;
        internal static readonly int AttributePointsPerLevel = 2;
    }
}

Attribute.cs

A simply attribute that stores it (Base)Value as well as a list of AttributeModifiers with both a flat (BuffValue) value as well as a coefficient, to get the total value one adds the base with all the buff values then multiply by its coefficient.

One idea here to optimize is to also store a private copy of both its total BuffValue as well as the total coefficient. That will give two more fields, and slower execution on adding and removing AttributeModifiers. While it will be faster to perform lookups.

using System.Collections.Generic;

namespace Character
{
    class Attribute
    {
        public int Value { get; private set; }
        public Dictionary<string, AttributeModifier> Modifiers { get; private set; }

        public Attribute (int value)
        {
            Value = value;
            Modifiers = new Dictionary<string, AttributeModifier>();
        }        

        public void Inc ()
        {
            Value++;
        }

        public void Dec()
        {
            Value--;
        }

        public void AddAttributeModifier (string name, int constant, float coefficient)
        {
            Modifiers.Add(name, new AttributeModifier(constant, coefficient));
        }

        public void RemoveAttributeModifier (string name)
        {
            Modifiers.Remove(name);
        }

        public int GetBuffValue()
        {
            int buff = 0;

            foreach (AttributeModifier m in Modifiers.Values)
            {
                buff += m.Constant;
            }

            return buff;
        }

        public float GetCoefficient()
        {
            float coefficient = 0.0f;

            foreach (AttributeModifier m in Modifiers.Values)
            {
                coefficient += m.Coefficient;
            }

            return coefficient;
        }

        public int GetTotalValue()
        {
            return (int)((Value + GetBuffValue()) * (1+GetCoefficient()));
        }
    }
}

AttributeUnmodifiable.cs

Immutable version of Attribute, to make sure that nothing can be changed outside of Character.

using System.Collections.Generic;

namespace Character
{
    class AttributeUnmodifiable
    {
        public int Value { get; private set; }
        public Dictionary<string, AttributeModifier> Modifiers { get; private set; }

        public AttributeUnmodifiable(int value, Dictionary<string, AttributeModifier> modifiers)
        {
            Value = value;
            Modifiers = modifiers;
        }

        public int GetBuffValue()
        {
            int buff = 0;

            foreach (AttributeModifier m in Modifiers.Values)
            {
                buff += m.Constant;
            }

            return buff;
        }

        public float GetCoefficient()
        {
            float coefficient = 0.0f;

            foreach (AttributeModifier m in Modifiers.Values)
            {
                coefficient += m.Coefficient;
            }

            return coefficient;
        }

        public int GetTotalValue()
        {
            return (int)((Value + GetBuffValue()) * (1+GetCoefficient()));
        }
    }
}

AttributeModifier.cs

namespace Character
{
    class AttributeModifier
    {
        public int Constant { get; private set; }
        public float Coefficient { get; private set; }

        public AttributeModifier(int constant, float coefficient)
        {
            Constant = constant;
            Coefficient = coefficient;
        }
    }
}

Test.cs

A (very) simple test to execute the code.

public class Test
{
    public static void Main()
    {
        Character c = new Character(5);

        System.Console.WriteLine ("Level: " + c.Level + " ( " + c.ExpCur + " / " + c.ExpTnl + " ) ");

        foreach (string s in AttributeConstants.AttributeNames )
        {
            System.Console.WriteLine (s + ": " + c.GetAttribute(s).GetTotalValue() + " ( ( " + c.GetAttribute(s).Value + " + " + c.GetAttribute(s).GetBuffValue() + " ) * " + c.GetAttribute(s).GetCoefficient() + " )");
        }

        System.Console.WriteLine ("Attribute Points: " + c.AttributePoints);
    }
}

Any feedback, be it just one line that could be changed is both wanted and appreciated.

Update(s) from comments:

You talk about optimizing things (attribute values), as well as the need to make sure that "nothing should directly be exposed".

Optimizing isn't that much of a concern know. I will know later on if that is needed, for now it is more the architectural part, is it good code praxis what I've done?

Are you primarily looking for review of the code's performance? Or security? > Or standards compliance?

Standard compliance.

Also, is this Character interacting with any other classes? Or are we to assume this is a closed system consisting only of the code given?

The character should only interact from the way the Test.cs is using it. There will no sorts of reflection or its like. Other systems will access it for information. They will always have to go trough Character.cs to change its values, after its rules.

Update(s) from answers:

Are Attributes things or values of things?

Solved, it is now more clear what they are(will) be used for in the code.

One man's Attribute is another man's AttributeUnmodifiable... Don't Repeat Yourself (DRY)

They are both solved by having an an abstract class Stat backing them up now.

C# is not Java: Use first class properties and leave the getters at home!

This code is being translated from Java... that obvious? :(

\$\endgroup\$
  • 1
    \$\begingroup\$ You talk about optimizing things (attribute values), as well as the need to make sure that "nothing should directly be exposed".. Are you primarily looking for review of the code's performance? Or security,? Or standards compliance? Also, is this Character interacting with any other classes? Or are we to assume this is a closed system consisting only of the code given? \$\endgroup\$ – Tersosauros Feb 11 '16 at 18:34
  • \$\begingroup\$ @Tersosauros I hope I added some clarifications now for it. If there still are questions or areas that are unclear feel free to ask for further clarification. \$\endgroup\$ – Emz Feb 11 '16 at 18:42
  • \$\begingroup\$ Is AttributeConstants just for testing purposes, or would this be how you intend to store these values permanently? \$\endgroup\$ – Tersosauros Feb 12 '16 at 5:38
  • \$\begingroup\$ Permanently, or well, they might be moved to a some sort of local DB. \$\endgroup\$ – Emz Feb 12 '16 at 8:20
  • \$\begingroup\$ Well, if they might be moved to anywhere else, I would recommend you encapsulate this with a little more OO. Something like an IAttributeProvider interface, which you can then have a simple internal implementation like the current one and a more complicated DatabaseAttributeProviderImpl or similar for when your solution architecture (inevitably) becomes more complicated. \$\endgroup\$ – Tersosauros Feb 12 '16 at 10:01
6
\$\begingroup\$

Are Attributes things or values of things?

In your current naming/abstraction/etcetera, it seems an Attribute is simply an encapsulated integer value (albeit with some in-built formulas attached in the form of Modifier's). It strikes me as odd that an Attribute doesn't have a Name, for example. (Hence why it seems more like an encapsulation of integer than an abstraction of a game Characters' attribute.)

I think fields such as Name, Description, etc could be added to Attribute. (It is currently unclear what the AttributeDescriptions in your AttributeConstants class are for, IMHO.)


One man's Attribute is another man's AttributeUnmodifiable...

First of all, let me just rant for a moment... AttributeUnmodifiable!? Surely under any reasonable naming convention this should be called an UnmodifiableAttribute? End class names with a noun. Phew! Okay </rant>.

It seems to me that what you are representing with an Attribute changes slightly. In particular, inside Character you essentially want Attributes to be mutable, while outside you want them immutable (or "unmodifiable", as your code puts it).

Given that (as stated in your edits) you're not concerned about reflection/introspection/reverse engineering/other security issues.. It would make more sense to use an interface to accomplish this immutability.

If you had an IAttribute interface, with all of your public properties and methods defined, and then an Attribute, MutableAttribute or even AttributeImpl class implementing that interface - you could make all of the public methods and properties of Character return IAttributes. This way, Character can internally know about the internal public details implemented by an Attribute and outsiders only see the (immutable/unmodifiable) mask.


Don't Repeat Yourself (DRY)

Even if you elect not to create an IAttribute interface, it still seems that there's a lot of repeated code shared between Attribute and AttributeUnmodifiable^. For example, the GetBuffValue, GetCoefficient and GetTotalValue methods have *identical* implementations in each class. This makes maintenance and upkeep far more time consuming (as code changes now have to occur at two points).

^ Note: Making that IAttribute interface and making an AbstractAttribute are neither mutually exclusive, nor co-dependant. you can do either one, or both (or neither, if you want).

For example, let's say some astute programmer pointed out that you should be using double-point precision for the accumulator variable in GetCoefficient. At the moment, you would have to make this change in the GetCoefficient implementation in Attribute, test it, etc, then make the same change again in AttributeUnmodifiable (and test it again, etc).

Enter AbstractAttribute

Rather than have all of this repeated code laying around, you could implement it once (DRY), and then have Attribute and UnmodifiableAttribute (aka AttributeUnmodifiable) extend AbstractAttribute to get the (single, easy to maintain) implementation.


C# is not Java: Use first class properties and leave the getters at home!

While we're on the subject of the GetBuffValue, GetCoefficient and GetTotalValue methods... Let me just have another little rant regarding naming conventions. C# is NOT Java, C# has first class properties (i.e. a property in C# isn't just a method with a certain word at the start of the name). Java-style getters are NOT the way we do things in C#! </rant>

Instead of:

public float GetCoefficient()
{
     ... getter code...
}

(Which, as indicated by the () brackets at the end of the name, declares a method.)

Try using a property (without the Get at the front):

 public float Coefficient
 {
      get
      {
           ... getter code...
      }
 }

The same goes for GetBuffValue and GetTotalValue too.

In general, if something is just an enquiry about an objects state, and doesn't require any arguments and will not change the object's state - use a property (not a method).


Don't return state from setters, particularly in a language with first class exceptions

Lastly, I've noticed that the AddExp method of your Character class is returning a value (that value being the current level, indicating if the increase in XP led to a level-up). As pointed out over here, such a thing violates the principle of Command–query Separation (CQS).

If a method that is responsible for mutating state (a setter in Java parlance), ever needs to return something - then usually that is indicative of an error occurring. In C#, we have Exceptions for this - so setters should really NEVER need to return.

I understand the need for the game engine (or whomever is using the Character instance) to be notified when a level-up occurs, however, there are other (better) ways of doing this (which don't violate CQS).

The pub/sub or "Publish/Subscribe" pattern comes to mind here. (The Character can notify subscribers of a 'level-up' Event, etc). This way, the logic to detect a level-up lives in one place (character), rather than needing to be everywhere that AddExp is called.

\$\endgroup\$
  • \$\begingroup\$ I don't have time to go through it all now, I read it. This is the feedback I wanted. Two fun things, I thought I overused Properties, I thought that is where the attack were going to be aimed at. Second, I asked myself how hard I would be criticalled by the "Attribute vs AttributeUnmod...". I just did not know a proper way to solve the later. Again thanks. Will get back with more substance to my comment later this day. \$\endgroup\$ – Emz Feb 12 '16 at 8:26

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.