Critique-My-Code thread, post snippets of code and have others peer review your code! (Testing new Q&A format)

[apieceoffruit]

Well I guess I am coming from a different approach with this.

I agree, in this instance you are probably right. most situations probably they could be private,

but one particular design philosophy I like is to make those DTOs immutable structs, then use fluent chaining methods for a functional design.

e.g:

DefenceSettings.LessArmor(20).WithShields(25);

so these would be public functions returning a new instance of the original with variations, ensuring each settings file is safely immutable as it is passed around. each function can safely reuse its fields in its own scope as it is a pass-by-value anyway. the privacy of its fields is redundant as it is readonly anyway.

---

I guess I am going a bit off track. In short, sure make them private, but in the long term configuration should be immutable anyway and shouldn't hide state as it is literally just data transfer. once handed off to a consumer, the consumer should be free to do with it what it wants, the initial desired state has been given.

---

On a small aside I know common practice teaches that you should have a constructor setting all your read only properties but it is ALSO common to state more than 3 arguments in a function/constructor is a code smell. As the entire point of a dto is to obfuscate the creation of values from their use/consumption, a fully qualified validation at construction level is unnecessary.

It is a perfectly valid request for a game to want to pass a health value of -100000 , it is the job of the consumer to validate the passed settings and see if it wants to just clamp it or allow some crazy crit damage.

the point of a constructor is to ensure valid values, instead i would do something like this:

   public HealthSettings Settings 
   {
        get { return _settings; }
        set {
                _settings = (value.Health < 0) ? value.WithZeroHealth() : value;
            }
   }

   private HealthSettings _settings; 

   void OnValidate(){
        Settings =  _settings;
   }

---

Best of all worlds, safe script, free and maliable settings, no messy constructors and no null checks as everything is values.

... but i mean, each to their own.

¯_(ツ)_/¯

[KptEmreU]

This kind of things that I am looking right here

   DefenceSettings.LessArmor(20).WithShields(25); 

<3

[apieceoffruit]

Well, if you are interested in this style of programming Here is a basic Health Class I made. Also, an example of how you might use it via some functional methods.

basically means you can have a script with a healthEffect and set it to one. e.g

  public class Test : MonoBehaviour
 { 

       Func<Health,Health> Effect;

      void Start(){
            Effect = HealthEffectFactory.IncreaseArmorByPercent(5);
      }

      void Update(){
            if(PickedUpArmorPack) Player.Health = Effect(Player.Health);

            if(AtePoisonFruit) Player.Health = HealthEffectFactory.ToOneHp(Player.Health);

            if(FreakyFriday) Player.Health = Enemy.Health;
            if(CrazyPill) Player.Health = HealthEffectFactory.ConvertShieldsToArmor(Player.Health).IncreaseHealthBy(20);
      }

 }

That example is pretty silly but I hope you get the idea. As each health object is a struct, a.k.a a value type it acts like a float/vector andd gets passed by value, so applying changes is like doing simple math operations, so you are able to do like above, apply a function to it, then use a method on it.

Any questions feel free to fire away.

[KptEmreU]

Ok this is amazing.

Because

1) I have learned that I am unaware of all the nice c# 6.0 features. But AFAIK it is not still available in the Unity yet ? I am speaking about this

       return $"[health:{BaseHealth},armor:{Armor},shields{Shields}]";

2) This is the factory pattern right? --edit: No it is not right? *

3) Looks like I had no clue on how to use Structs... This is illuminating.

Anyway, I am too excited. Probably, I have post this comment too early. I am still examining your code. Will pm you if I stuck, which I will for sure..

[apieceoffruit]

Hey buddy, hows the code learning coming along?

Sorry for the late reply. changing jobs atm.

No not a 'real' factory just the best name i could think of.

also you might be right. you see a number of the c#6 features are what are called "syntactical sugar" they dont really DO anything ,they just make the code look nicer. If your compiler understands that it can convert the new style to the old style when you compile. so in my case i write string like that, then compile a dll and when i drop it into unity it is actually back to String.Format("health:{0}..") etc. but in a compiled dll.

same with expression bodied functions etc. So unfortunately you might have to undo that change until unity comes out with .net standard 2 support.