3
\$\begingroup\$

So I am working on a small RPG in Unity. Right now I am writing some business classes, POCOs, that have no link to Unity except being in the required C# flavor and using Debug.Log and stuff.

I have been doing a lot of Java for years and some C++11, but no C#, so some concepts are not familiar to me.

I think most of the code is so-so, I'd like some advice, maybe someone might see future issues or stuff like that.

Entity class :

using UnityEngine;
using System.Collections;

public abstract class Entity {

    public LevelingManager LevelingManager {get; set;}
    public EntityStats EntityStats{get; set;}
    public int CurrentLifePoints{ get; set;}
}

Fighter class :

using UnityEngine;
using System.Collections;

public class Fighter : Entity {

    public Fighter(){

        this.EntityStats = new FighterStats ();
        this.LevelingManager = new FighterLeveling (EntityStats);
    }

}

EntityStats class :

using UnityEngine;
using System.Collections;
using System;

public class EntityStats : IComparable {

    public int PhysicalForce{ get; set; }
    public int PhysicalDefense{ get; set; }
    public int MagicalDefense{ get; set; }
    public int MagicalForce{ get; set; }
    public int Speed{ get; set; }
    public int Life{ get; set; }
    public int Level{ get; set; }

    public EntityStats(){
        Level = 1;
    }

    #region IComparable implementation
    public int CompareTo (object obj)
    {
        if (null != obj && obj is EntityStats) {

            EntityStats stats = obj as EntityStats;

            if (stats.Level >= Level
               && stats.Life >= Life
               && stats.MagicalDefense >= MagicalDefense
               && stats.MagicalForce >= MagicalForce
               && stats.PhysicalDefense >= PhysicalDefense
               && stats.PhysicalForce >= PhysicalForce
               && stats.Level >= Level
               && stats.Speed >= Speed) {
                return 0;
            }
        }

        return -1;
    }
    #endregion
}

Fighter stats :

using UnityEngine;
using System.Collections;

public class FighterStats : EntityStats {

    public FighterStats(){

        PhysicalForce = 10;
        PhysicalDefense = 10;
        MagicalDefense = 5;
        MagicalForce = 0;

        Speed = 5;
        Life = 10;
    }


}

LevelingManager class :

using UnityEngine;
using System.Collections;

public abstract class LevelingManager {

    public int ExperiencePoints{ get; set; }
    public EntityStats ComparableStats{ get; set;}

    public int ExperienceNeededForNextLevel (){
        return  Mathf.FloorToInt (Mathf.Log (ComparableStats.Level + 1) * 100);
    }

    public LevelingManager(EntityStats comparableEntityStats){

        ComparableStats = comparableEntityStats;
        ExperiencePoints = 0;
    }

    public void AddXP(int points){

        int threshold = ExperienceNeededForNextLevel();

        if (ExperiencePoints + points >= threshold) {

            int remainder = points - threshold;
            LevelUp (remainder);

            //FIXME be careful with recursion .. (small stack on some systems ?)
            if(remainder > 0)
                AddXP (remainder);
        }
        else {
            ExperiencePoints += points;
        }
    }

    protected virtual void LevelUp(int remainder){
        ComparableStats.Level++;
        ExperiencePoints = 0;
    }


}

Fighter leveling :

using UnityEngine;
using System.Collections;

public class FighterLeveling : LevelingManager {

    public FighterLeveling(EntityStats comparableEntityStats) : base(comparableEntityStats){
    }

    protected override void LevelUp(int remainder){
        base.LevelUp (remainder);

        int adder = Mathf.FloorToInt (Mathf.Log (ComparableStats.Level + 1));

        ComparableStats.PhysicalForce += adder * 3;
        ComparableStats.PhysicalDefense += adder * 3;

        ComparableStats.MagicalDefense += adder;
        ComparableStats.Speed += adder;
        ComparableStats.Life += adder * 2;
    }
}

Stuff Class :

using UnityEngine;
using System.Collections;
using System;

public abstract class Stuff {

    public EntityStats RequiredStatsToUse {get; set;}
    public string StuffName { get; set; }

    public Stuff(EntityStats stats, string stuffName){
        this.RequiredStatsToUse = stats;
        StuffName = stuffName;
    }

    public bool CanBeUsedBy(Entity entity){
        return this.RequiredStatsToUse.CompareTo(entity.EntityStats) == 0;
    }

}

WoodenSword class :

public class WoodenSword : Stuff {

    public WoodenSword() : base(new EntityStats () , "Wooden sword"){

        RequiredStatsToUse.PhysicalForce = 10;
    }
}

Tests :

    [Test]
    public void should_go_to_level_2()
    {
        var leveling = new FighterLeveling (new FighterStats());
        leveling.AddXP (75);

        Assert.AreEqual (2, leveling.ComparableStats.Level);
        Assert.AreEqual (6, leveling.ExperiencePoints);
    }

    [Test]
    public void should_go_to_level_5()
    {
        var leveling = new FighterLeveling (new FighterStats());
        //should be 478, but there's a small diff due to the rounding
        leveling.AddXP (476);

        Assert.AreEqual (5, leveling.ComparableStats.Level);
        Assert.AreEqual (0, leveling.ExperiencePoints);
    } 

    [Test]
    public void object_should_be_usable_by_a_level_1_fighter()
    {  
        var fighter = new Fighter ();

        var weapon = new WoodenSword ();

        Assert.AreEqual (true, weapon.CanBeUsedBy (fighter));
    }

    [Test]
    public void object_should_not_be_usable_by_a_level_1_fighter()
    {
        var fighter = new Fighter ();

        var weapon = new WoodenSword ();

        weapon.RequiredStatsToUse.MagicalForce = 10;

        Assert.AreEqual (false, weapon.CanBeUsedBy (fighter));
    }
\$\endgroup\$
  • \$\begingroup\$ Why do you need EntityStats : IComparable ? When are you going to compare the user's stats as a whole ? \$\endgroup\$ – Denis Aug 18 '16 at 10:49
  • \$\begingroup\$ Just look at the Stuff class, this is used as an abstraction of stats to compare required stats for a weapon and the stats from an entity. Well, the name may not be the best.. Just "stats" might be a better name \$\endgroup\$ – Hello_world Aug 18 '16 at 11:12
  • \$\begingroup\$ Alright that's fair enough. \$\endgroup\$ – Denis Aug 18 '16 at 11:16
2
\$\begingroup\$

Few notes :

  1. public abstract class Entity this should be an interface it provides nothing more than just the signature of some properties. But why do you need so much abstract classes in the first place ? They don't even have any abstract methods/properties in them, you can virtual functions in a non-abstract class.

  2. I don't see any problem with the recursion in the AddXP function.

  3. public int ExperienceNeededForNextLevel (){
        return  Mathf.FloorToInt (Mathf.Log (ComparableStats.Level + 1) * 100);
    }
    

    This function just returns some value it looks and sounds more like a property.

  4. Stuff doesn't sounds like a good name.

  5. You got some magic numbers in FighterLeveling I'm not sure why you multiple by 2/3, maybe declare them as const variables with appropriate names.

  6. In your Entity stats' CompareTo function you can shorten your if

    if (null != obj && obj is EntityStats) {
    
        EntityStats stats = obj as EntityStats;
    }
    

    You can just declare the variable outside the statement, give it the value of obj as EntityStats if obj is not an EntityStats it will return null

    EntityStats stats = obj as EntityStats;
    if(stats != null)
    {
    
    }
    
  7. You might want to restrict some of your properties so they can't be set outside of specific classes using private set, because if you have a function TakeDamage() and you perform some animations, some other calculations and also decreasing your player's health in there you definitely don't want to be able to just modify the property Health and instead you want to use the function right ? So restricting your variables is essential.

I'm not sure about the way you have your statistics. As far as I can tell you are still not at that stage where they actually benefit you but there are some important questions you should ask yourself before continuing because they will change most of your current code. Are you able to increase them by leveling up and putting points into them ? Are they providing just flat dps increase, flat health increase or they work on percentages ? Does your character has base stats and by obtaining more PhysicalForce you just add it to his current ? Depending on those answers you might need to construct a Stats class which holds all the useful info like PointsSpentInStats, AmplifierPercentage, etc. and if you don't do that now you will need to restructure a lot of your code later on.

\$\endgroup\$
  • \$\begingroup\$ 1) I mostly have abstract classes because I don't want them to be instantiable, is there a better way to express this in C# ? As for the Entity class, you are right, there is no escuse here ! / 4) I could not name it "Object" :) / 5) They are the coefficients used to add points when the fighter levels up / As you can see in the FighterLeveling class, the increase of stats is done by adding log(level +1) point to each stats; Then I apply a coefficient for each stat. (A fighter have a higher increase in Physical Force for example) I agree with the other points ! \$\endgroup\$ – Hello_world Aug 18 '16 at 12:03
  • \$\begingroup\$ As I said you should use Interfaces if your current abstract classes offer nothing more than just a signature of methods and properties, interfaces are also not instantiable. As for stuff you can call it Item for example ? I did some explanation on interfaces on this question you might find it useful codereview.stackexchange.com/questions/136647/… \$\endgroup\$ – Denis Aug 18 '16 at 12:12
  • \$\begingroup\$ Yes, as I said you are right for Entity, but the other abstract classes do have concrete methods ! You told me I had too much abstract classes, but I feel the other one (other than Entity) are legitimate, am I right ? \$\endgroup\$ – Hello_world Aug 18 '16 at 12:39
  • \$\begingroup\$ We can say that it's alright yes, however it can still be a normal class take a look at this post programmers.stackexchange.com/a/106603 . What am trying to say is that the usual abstract class doesn't implements all the functions and properties it has, that's more appropriate for a normal class nothing stops you from using abstract class anyway it's not that big of a deal. \$\endgroup\$ – Denis Aug 18 '16 at 12:51
1
\$\begingroup\$

IComparable

The way you've implemented IComparable is confusing and couples its implementation to its expected usage. CompareTo is supposed to return one of three values, to indicate <,==,> the item being compared to. Currently yours only returns 2 values, it's either the same (0), or it's less than. This is confusing because it means that the result of:

stats.CompareTo(statsHighLevel)

and

statsHighLevel.CompareTo(stats));

Are not always the same, which is akin to saying whilst stats==statsHighLevel, statsHighLevel!=stats.

This can be demonstrated with a simple test:

[Test]
public void entitystats_compareto_not_reversible()
{
    EntityStats stats = new SomeTests.EntityStats();
    EntityStats statsHighLevel = new SomeTests.EntityStats();

    statsHighLevel.Level++;

    Assert.AreEqual(0, stats.CompareTo(statsHighLevel));
    Assert.AreNotEqual(0, statsHighLevel.CompareTo(stats));
}
\$\endgroup\$

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.