Question Excessive use of properties bad practice?

Here is an example of the smallest script I can find at the moment for what I mean about properties. The purpose of using the getters for Mana and Health is simply just to update stuff like UI and maybe a use case for when the player dies. However, there are times when I feel like making adding these properties just to be a middle man for a variable seems like bloat. I'm writing extra lines of code simply to hold one variable. I was wondering if this is the proper way to use getters? In this case the alternative would just to make CharacterStats public and save myself many lines of code. I would have to type out Character.Stats.Health however vs Character.Health.

Code (CSharp):

1. public class Character : MonoBehaviour
2. {
3.     [SerializeField]CharacterVisual Visual;
4.     private CharacterStats Stats;
5.     public int Mana => Stats.Mana;
6.     public int MaxMana => Stats.MaxMana;
7.     public int Health => Stats.Health;
8.     public int MaxHealth => Stats.MaxHealth;
9.  
10.  
11.   public void Initialize(CharacterStats stats){
12.         Stats = stats;
13.         UpdateVisual();
14.     }
15.     private void UpdateVisual(){
16.         Visual.UpdateVisual(Stats.Health,Stats.MaxHealth);
17.     }
18.     public void TakeDamage(int value){
19.         Stats.Health-=value;
20.         UpdateVisual();
21.     }
22.     public void Refresh(bool refreshFull = false){
23.         Stats.Refresh(refreshFull);
24.     }
25.     public void TakeMana(int value){
26.         Stats.Mana-=value;
27.     }

 

The first thing I would ask yourself is, "What value does this bring?"

The answer may be that Mana is more clean than Stats.Mana.

But the other problem is now you have sheets of extra code to maintain, possibly in many places.

If anything, the Character should not even be AWARE of the specific contents of Stats but rather access the data within Stats via dedicated accessor methods.

Proper OO says you should make GetMana(), GetMaxMana() etc. methods in the Stats class and use those.

 

This is more of a design question than about properties. Should users have to remember it's

c1.stats.health

, or should there be a "wrapper" shortcut --

c1.health

or

c1.health()

(depending on whether you like properties).

On one hand, adding reach-through shortcuts is a thing. In a big program, with lots of obscure classes, remembering that

c1.stats

contains health and so on can be a time-waster. Being able to type c1-dot and see dot-manna (or dot-manna()) would be nice. But adding those shortcuts also takes time, and may be pointless. You may often need to think about the whole

c1.stats

. In that case,

c1.stats.manna

is intuitive, and an extra

c1.manna

is more confusing.

A big factor is old-fashioned encapsulation. Do you think

stats

might move or change? In that case, you'd have to change every user to

c1.stats.magic.manna

or whatever. Making a nice c1.manna interface (and again -- property or function) solves that problem. But then again, games aren't that big -- find&replace will work fine.

 

What does C1 refer to?

 

I assume that I should also make a SetMana in the Stats class as well if I go with that option?

What would the code for referencing the variables look like in practice?

Combat Class
Original:

Code (CSharp):

1. public bool canPlay(Card card) => Character.Mana >= card.Stats.Cost;

New:

Code (CSharp):

1. public bool canPlay(Card card) => Character.GetMana() >= card.GetCost();

Original:

Code (CSharp):

1. if(Player.Health == 0||

New1:

Code (CSharp):

1. if(Player.GetHealth() == 0)

New2 :

Code (CSharp):

1. if(Player.IsDead()) // this is from Stats.IsDead() which just means the Health is 0

Removed The Getters and allow other classes to read the Stats if they need the variables. When modifying something it will need to go through the character class because I want the visual feedback to occur.

Code (CSharp):

1. public class Character : MonoBehaviour
2. {
3.     [SerializeField]CharacterVisual Visual;
4.     public CharacterStats Stats;
5.     public void Initialize(CharacterStats stats){
6.         Stats = stats;
7.         UpdateVisual();
8.     }
9.     public void UpdateVisual(){
10.         Visual.UpdateVisual(Stats.GetHealth(),Stats.GetMaxHealth());
11.     }
12.     public void TakeDamage(int value){
13.         Stats.TakeDamage(value);
14.         UpdateVisual();
15.     }
16.     public void TakeMana(int value){
17.         Stats.TakeMana(value);
18.         UpdateVisual();
19.     }
20.     public void Refresh(bool refreshFull = false){
21.         Stats.Refresh(refreshFull);
22.     }
23.  
24. }

 

My main objection to all the

public foo => stats.foo;

nonsense is that it's just more lines of code you can ignore, more boilerplate that MUST be there, MUST be perfect, WILL have bugs, WILL get out of date, and then generally gets ignored. Meh.

That should probably be an observer pattern: the Character registers with the Stats instance and says "tell me when stats change" and then it does the appropriate thing.

Or if it's just for feedback, have a feedback system that monitors the stats, when they change is pokes the character to make them give visual feedback.

You can really go down this OO rabbithole as deep as you like, and all of this stuff can seriously drive you nuts unless you just do the bare minimum necessary to address the pain points you feel, refactoring as you go along and learn more about your problem space.

As Owen points out, games are often small enough in scope that you can throw away all this compsci major OO partitioning stuff and just put the stats right in the character and simplify everything by about 10x.

 

I concur with all of this. If you a have a stats collection design, stick with that. That pretending facade is just painfully redundant and will bite you or waste your time.

Btw, if stats object is something only you will use and it's just a data holder, make it public floats. If you have reasons to validate on input or you are not the user (and there is some logic going on), make public properties (or just getters to safeguard).

Next, will this design change? Are you going to add/remove stats? Probably. Never make stuff you'll heavily depend on only because of convenience + you're repeating yourself. That's two violations in my book.

 

I do believe you answered your own question. You've called it "excessive" from the get go, and you've given two perfectly valid reasons for that, and seem unsure of the benefits.

In cases like this I think about what might go wrong if I change things to be less repetitive. What would be bad about just exposing CharacterStats itself? Would that give other objects access to change stuff that they shouldn't? Would it increase maintenance load because CharacterStats changes a lot? Would it couple stuff that you really don't think should be coupled? Would it make another team member / your boss grumble about stuff you don't want to waste time arguing about?

After thinking about that, if you've got good reasons for it being how it is then those 4 lines of fluff might not be fluff. (Though you could also think about whether there are better ways to satisfy the good reasons you come up with.)

Otherwise, go with your gut. Have a go at nerfing them and exposing CharacterStats. See how it goes. If it sucks you can always put it back. And either way you'll have a bit more experience to refer back to next time you need to make a decision like this one.

Yeah. Without knowing more about the specific case at hand, this case makes me think that CharacterStats should present itself as immutable (i.e. only read-only properties exposed) and have a MutableCharacterStats sub-class or similar. That way Character can have a MutableCharacterStats so that it can change them on level up or whatever, but it can expose it to other objects as a CharacterStats object so they can't accidentally mess with it.

 

That's not even remotely correct.

Properties and their accessors is not a hard topic, it is basic C# -- and prefixing functions such as GetMana and SetMana is only done in Java etc.

https://learn.microsoft.com/en-us/d...ng-guide/classes-and-structs/using-properties

 

When you mention a feedback system, Im assuming some kind of listener? Subscribe the events to this feedback system like Action<CharacterVisual>OnDamage then when it procs it will do something to that character?

 

It was actually YOU who mentioned the need for feedback:

I was simply pointing out that such a system would be better served with a different approach.

This actual statement I made:

implies a monitor... but you could do it with a callback too.

 

Stepping back, this topic is just messing around with unimportant details. You asked about properties, and that's the advice you got. But an easy rule is to never use them -- leave variables as public, or else write a real function like takeDamage(). Times when a property is actually a good option are rare, and even then, just writing functions is only about 2% worse.

And don't use the => function thing. Write all functions with {} bodies. It's pretty much the same logic. => hardly saves you any time, but wastes time trying to decide if a function is "short" and if it will stay "short".

And for design in general, at first it's fine to just write some junk to make the stuff happen. "Bad design" only shows up when a program gets large, and depends on the exact way it gets large (which you don't know yet). Plus 1/2-decent design is pretty basic -- when you can, break basic ideas into classes and try to hide details through functions.

 

A note for anyone finding this via search: Most advice given here is for "wanting to write basic Unity games fast" but are horrible advice if you are looking at working as a developer for real.

 

care to define?
I suddenly felt unreal for a moment. It was almost as if you were wielding some sort of a cap of magical authority.

 

Try applying to a job for any larger firm and declare that you don't like to use properties and just put variables as public, you will not get hired.

Separations of concerns is an basic design aspect, and not giving outside units the access to edit data it should only be able to read is important. Very important.

You can write snide replies and pretend it isn't the fact, and feel smug that you have done so, but you will not be giving good advice to future developers that visit these forums tomorrow or later.

 

@Zarkow
To defend my position, I would have to write a really long article explaining what game development is and what isn't. It's ironic that you would nitpick and call others amateurs, yet you sound like you're a long way from the actual domain of this problem.

Yes properties have their merits, and I use them all the time, but my advice is hands down legit professional advice. There is no recommendation that stops anyone from using public fields for this case scenario, and in fact I bet I could find official recommendation against anything more complicated than that in this case scenario.

Additionally, there are many recent and sweeping changes in how games are developed, and how rapidly they are developed: there is nothing to gain by facading a class like this, and I'm an old guy, I've seen all kinds of patterns and bullshit.

Programming defensively like people are going to launch nuclear weapons with the APIs we write is so 00's, but contrary to some weird beliefs out there especially within the structures of management, programming is supposed to be more about tight collaboration than about guarding from the future replacement of oneself in a company. The only place where you advice is ok is some sort of B2B API, a module, a service, or a framework, where you're actually making sure that people won't somehow abuse or invalidate the internal state of a solution.

A video game, however, is a self-dependent software package that should be open to itself, and never hide anything from itself, regardless of how many tiers of interconnectedness there are. This is what good system design is for. Doing otherwise only multiplies the original problem, which is already crazy complicated, and needlessly chokes what was supposed to be an open bandwidth.

As I said, this example explicitly multiplies the work now as well as in the future and violates DRY, without benefits. That's much worse than not following things by the book.

But let's just say you win because I really don't have time for this.

 

Anyone can write their small games just the way to want -- but when working in larger teams or when transitioning to professional teams they will absolutely be required to adhere to good design patterns and structure their code in recommended ways.

To claim that a game is 'open to itself' and therefor everything can be accessible from everywhere is to set up oneself for spaghetti-code scenarios and amazing cross-dependencies, or code that is really hard to test.

Next you will claim that unit-test, integration-tests and code-reviews are only done by crazy people too. Cool.

This topic should NOT be filled with advice that one shouldn't lean into doing things the proper way, should NOT teach anti-patterns (writing your own Get and Set accessors) because it WILL confuse the ones that are more likely to read them - those that google because they are towards the less experience side of the scale.

It MAY point out that for minor code-units it is often a quick help, initially, to either set run-time properties to Public or to make extensive use of [SerializeField] private <type> _variableName to allow the Unity editor to visualize the data for debugging-purposes, but that you only do these things with these targets in mind and often go back and refactor the code to a better state once the issue is solved, the solution implemented or you have an ingame debug-UI that replaces the Unity properties tab for run-time objects.

For someone that finds this thread in a few years -- do things right, or be very aware of when you are purposefully stepping away from it and leave a comment in the code to your fellow programmer explaining why you did something that way -- even if that person is yourself some weeks later.

 

From a purely functionality standpoint I don't see a problem with either of these as long as it's consistent with the rest of your code and it's property documented. With regards to bloat my only concern is whether it will negatively impact IDE autocompletion. Too many entries can make it difficult to discover what you need.

 

A part of good system design is also deciding how things should interact with one another. What's being discussed here is mostly tools and practices to communicate and enforce that design.

While that's true, that's just for this example. Jumping from there to "never hide anything" just steers things towards a different set of problems.

I also suspect that each of us is interpreting the original example differently. Whether or not it's reasonable to expose CharacterStats depends on a number of design considerations elsewhere, of which we know nothing.

Very fair call. My initial reaction to your first post was to think "yeah, but only one person actually said that". But on a quick re-read, for someone who's asking these questions many of the answers could be interpreted this way, and it's worth clarification.

So on that note... when I suggested "exposing CharacterStats itself", this was with the assumption that CharacterStats is itself designed reasonably well. For instance, character.Stats.MaxHealth would be a read-only property, and the owning character would have some other mechanism to make valid changes.

 

With all this talk about general design, two things occurred to me. One is that the

Stats

class doesn't need to exist -- having health and manna directly in the Player class seems more normal. The other is that the Player class probably shouldn't have too many member functions. It can be mostly a bag of variables, letting the game-runner class play with them. Reasoning:

Stats would be a good class if it was shared among several other classes that needed it. But it's only going to be used in this one place (even if you summon monsters which also have health and manna, the way they work will be very different). But no problem -- it's fine to create component-type classes to break up a too-large class. For example, if animating the player's face icon requires a dozen variables for timing and you keep mixing them up with other variables, create an iconAnimation class. You can probably make it a real class, too -- all private fields and a few easy-to-understand functions to call.

But none of that happens here. Health and manna are prime variables in the player, and you're already touching them directly. Instead of writing

if(stats.manna==stats.maxManna)

you may as well allow yourself to use

if(manna==maxManna)

. Or if you were going to change that to

stats.isFullManna()

it can now be the easier to read

isFullManna()

.

As for making Player into a "do stuff to me" class instead of a "ask me to do stuff to myself" one -- it's a card game. Everything in a card game touches everything else. It's impossible to make a nice, self-contained class. Sure, hide what you can under a nice interface, but the rest will be super-leaky OOP violations -- the manna cost of some cards depends on how many puppies are on the board, and puppies damage depends on the # of kittens in the other player's graveyard, and you taking damage triggers card X to do Y ... . This means that calling your

Player[0].canPlay(card0)

requires the player class to be able to reach into the game board, the other player, the discards ... everywhere. You're going to hack that adding statics and having the player use

gameRunner.GR.board

to examine the board for those puppies. That's the opposite of a good OOP member function.

You may as well advertise how every function needs global knowledge by having them be in gameRunner. Rewrite

canCast

as a normal game-runner function:

if(canCastCard(Player[i], card1))

. It isn't so confusing when it starts using

board.totalMonsters()

and

GraveYard[1-curplayer].depth

.

 

When someone asks you where the railroad station is, do you also recommend them to look left and right when crossing the street, because there is a good chance they'll get hit by the car on the way there?

In fact, we can even look up for the chances, are lo and behold, they're greater than zero!

For someone that finds this thread in a few years.

I find your advice completely artificial, and your zeal amazing. Whoever is so below the general advice that's been given by me and Kurt (et al), and heading right to spaghetti code, should not ask about coding styles, properties and whatnot. This isn't a catch all thread, the OP had a very specific question and we gave him our professional opinions, very local to the OP's intention, which is the overall design and maintenance practice. It was not an open invitation for ideologues to come and preach about the academic purity of some ideal code out there.

I even went further and explained the rationale behind my opinion, you have completely skipped the part where I say that I have experience with what you're saying is preferential and it is not, it chokes the project, turns it into a bureaucratic hell. Nothing is black & white in programming, just like in any other artificial system of haphazard rules.

I am a technically inclined person, who also happens to have a lot of experience with all kinds of codebases and I've seen a fair share of what people do. And let's not mention GitHub. I've seen a lot of it, and the winners have just one style in common: they all write compact, clean, readable, non-redundant, and pragmatic code. They use their tools, they are not the tools themselves.

You cannot help a paralyzed person win a marathon, but oh boy you can feel good for winning a stupid debate over nothing.

Writing a public data class is not an anti-pattern. Are you telling us that you write all of your data classes with getters and setters? Why? Do you even know what properties are and what they're for?

But hey, let's just skip the long conversation on any of this -- just confirm one thing: Are you pro or against the OP's original example? Also please explain why. And we'll leave it that. Just stop preaching, tell us what you believe in, and that's fine.

Here's the example once more.

Code (csharp):

1. public class Character : MonoBehaviour
2. {
3.     [SerializeField]CharacterVisual Visual;
4.     private CharacterStats Stats;
5.     public int Mana => Stats.Mana;
6.     public int MaxMana => Stats.MaxMana;
7.     public int Health => Stats.Health;
8.     public int MaxHealth => Stats.MaxHealth;
9.  
10.  
11.   public void Initialize(CharacterStats stats){
12.         Stats = stats;
13.         UpdateVisual();
14.     }
15.     private void UpdateVisual(){
16.         Visual.UpdateVisual(Stats.Health,Stats.MaxHealth);
17.     }
18.     public void TakeDamage(int value){
19.         Stats.Health-=value;
20.         UpdateVisual();
21.     }
22.     public void Refresh(bool refreshFull = false){
23.         Stats.Refresh(refreshFull);
24.     }
25.     public void TakeMana(int value){
26.         Stats.Mana-=value;
27.     }

I expect you to enumerate the reasons and benefits to have such a code. Please do not speak broadly of patterns and anti-patterns. I wish you would address the issue at hand, out of common courtesy, instead of using strawman argument to show how Kurt, me, and other people are somehow charlatans. While you're literally someone with zero background on this forum.

 

... asks about bad practices, then gives the smallest example they could find.

 

Yes! That's what the question was about. It's not a random example, it's a concrete example. The OP asked specifically what would be bad practices for what he tries to achieve!

And this is what prompted a concrete pragmatic advice from some of us: "don't bother with making redundant clutter just because, keep your code straight". I can also add (for anyone "who finds this thread in a few years"): "Feel free to introduce a better, although more bloated design after you've found out that it is insufficient to your needs, not before".

I'm not sure what else you're reading into this. This is not a broad philosophical take on how computer programming should look or feel like, if that's what you're seeing. But ok.

Here's the broad takeaway, that has nothing to do with properties per se, but you can extrapolate a lot:
Programming is an iterative process. Given a sufficiently complex body of code, an error is inevitable. It is only a matter of when. There is no working commercial software product that does not have bugs, or even complex issues that can only be solved through refactoring. This is an axiom. With that in mind, the only PROVEN way to minimize errors and optimize productivity is to very early: a) reduce clutter, b) create visual (or other documentational) guides, c) enforce succinct and uniform coding styles, and generally embrace coding practices that look almost like children drawings, INSTEAD of pretending that some religious practice will magically save you and then bloat everything with nonsense.

Best practices, for all ages, put shortly:
1) Put semantics upfront (also name things appropriately),
2) Always test your assumptions (and allow for tests in general),
3) Use as little boilerplate as possible (that doesn't mean you should be clever about minifying),
4) Minimize tangle (aka practice separation and independency),
5) Allow yourself to be able to replace parts without everything blowing up,
6) Do not repeat yourself (organize your code appropriately),
7) Sanitize all foreign input,
8) Boom, something magical happens and every design fits nicely almost without effort.

If public floats were "filthy" they wouldn't be an ordinary assortment of tools. There are zero official recommendations on whether to avoid public members or why they are bad. Nothing is bad in good hands. Business that behave this way simply employ bad programmers, and then their leads have to build the codebase defensively. That's not an excuse. Did you know that C# also has a goto? It's crazy but I actually got to use goto last week. This was a very sensitive mathematical algorithm, and goto was the best possible solution for it, that made it MORE readable and really clever, in a good way. Whaddya know. I can show it if you're interested.

 

If CharacterStats has more data/features and Character is exposed to classes that should not interfere with those then it makes sens that Character exposes only what's relevant.

However, one way to address this that i prefer is to use interfaces, so that you can expose only what is needed in a clean way for example:

Code (csharp):

1.  
2. public interfaces ICharacterStats
3. {
4.     public int Mana {get;}
5.     public int MaxMana {get;}
6.     public int Health {get;}
7.     public int MaxHealth {get;}
8. }
9.  

Code (csharp):

1.  
2. public class Character : MonoBehaviour
3. {
4.     private CharacterStats _stats;
5.  
6.     public ICharacterStats Stats => _stats;
7. ...
8. }
9.  

However, if CharacterStats only has what is exposed in Character then its pointless to expose properties.

If the reasonning was to plan for possibly swapping the stats class in the future then the interface way is likely the right way as well.

 

I'm not completely sure what this means, but I think I agree? Yes, that example is pointless. It lets us have several different "stats" classes which can be plugged into the player (an outsider can call

Player[i].Stats

, which returns any object with the interface ICharStats). That's pointless, right? We're never going to write a different class for health, manna, maxHealth and maxManna, then have some players use one and some use the other.

 

There could be additionals stats, specific to characters like force or moral, which arent required for buildings for example. While you still want a generic system for health so that your damage API works for all...

 

But

ICharacterStats

won't help with that. The main problem is it's easier for a Card game to handle player damage separately (with a IF or something). It's generally too different from summoned creature damage. But even if you wanted to do that -- to group players with monsters --

ICharacterStats

is on the wrong thing. We need the Player or Building to have the interface. It would be a basic

IDamageable

interface (google "unity damage interface" -- it's a popular topic). But then

ICharacterStats

4 read-only functions are wrong for that: why do we have manna? And why is health read-only? We need takeDamage(), isDead() and so on.

An interface written the right way to solve a specific problem is great. But so is a C# Dictionary. You wouldn't add a Dictionary to everything, would you?

 

To be clear, i didn't go as far as providing a ready to copy paste solution, merely suggested a way to structure it better.

Obviously just those 4 property make little sens. I believe in people and hope you're able to read between the lines a little.