Basic Player/Character Structure

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 Attributes = new Dictionary();
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 ex

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 .

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#!

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 su

StackExchange Code Review Q#119666, answer score: 6