Is it okay to make all variables public?

Question

I'm writing a simple turn based RPG system. I have a BattleScreen class that handles the drawing of well.. the screen. Thing is, it has to know pretty much everything about my Player class. Its Name, HP values, MP values, JobName, Level, Experience, Money etc.etc.etc. in order to write it down on the screen. My Draw() method ends up drawing pretty much every variable from Player, and it requires me to make pretty much everything in Player public

Which led me to believe it would be a better idea to just let the Player class draw itself, but then I end up with multiple Draw() methods that BattleScreen needs to call. A draw method in case the Player is the main player (which has all the data and names), and one where it's on the sidelines (with only the sprite and HP/MP visible), and other methods for other screens.

Both ways feel icky.

Is any of these two ways considered 'normal'? Is there a better way to design this?

Why not just expose the necessary values of the Player class by properties? — Tobias, Jun 18 '14 at 06:23
Fair enough, then my next question is; is it considered normal to have to expose every single property in my Player class? — Taelia, Jun 18 '14 at 06:26
I don't have any experience with xna, so this is more from a general C# stand point. If your class has values that need to be exposed, you should do so by properties, no matter how many there are. — Tobias, Jun 18 '14 at 06:30
From what I remember, if your class implements `IGameComponent`, then the default xna setup already handles some of the calls for you — Sayse, Jun 18 '14 at 06:33
You could make an interface `IDrawable` with some kind of `Draw()` method and have every class you want to be able to draw implement that interface. Then put all those objects in a `List` and whenever you need to draw them, loop over that list and call the method. --- but that way you'd be mixing *model* and *view*, which is usually a bad idea. — Corak, Jun 18 '14 at 06:34
I'd go with @Corak Keep all drawing logic of `Player` local in its own class. Moreover, I would make the interface method `Draw(ICanvas)` or something similar and have `BattleScreen` implement `ICanvas`. That way, you also don't have to expose details of `BattleScreen` to `Player`. — Vincent van der Weele, Jun 18 '14 at 06:44
@Taelia - "is it considered normal to have to expose every single property" - only the ones you want to be able to access from outside your class. But then, yes, it is considered normal to expose *properties*, usually in the "public get, private set" kind of way. — Corak, Jun 18 '14 at 06:54
Having `Player` draw itself would still end me up with all kinds of draw methods for different situations, right? One where the name is displayed above it and the hp bars below, one where the name is displayed to the side, one where only the hp and mp bars are visible, etc. all depending on when and where and in what menu the `Player` has to be drawn. — Taelia, Jun 18 '14 at 06:55
@Taelia - Indeed. You *could* extend the `Draw()` method (and taking @Heusters suggestion) to `Draw(ICanvas, IDrawingContext)` and deal with the different cases. But this would put more and more *view* logic into the *model* class. So you probably want to put that logic into separate [Object]View classes. For example `PlayerView`, which is basically a wrapper around an instance (object) of the model class, wherein you can deal with all the stuff related to drawing the object. You'd still need to expose the properties, but read-only exposing shouldn't be harmful, right? — Corak, Jun 18 '14 at 07:04

Christoph Fink · Accepted Answer · 2014-06-18T09:10:02.710

7

You should NOT make ANY member variables public, as this is considered bad practice.
You should use properties.

So instead of:

public string PlayerName;

you should use

private string playerName;
public string PlayerName
{ 
    get { return playerName; } 
    set { playerName = value; }
}

or the even shorter

public string PlayerName { get; set; }

You can then also make the property "read-only" from the outside like Corak already mentioned:

public string PlayerName { get; private set; }

Then from inside your Player you can get and set the value of PlayerName, but from outside, like in your Draw method, you can only get it. You can also later extend the getter/setter with additional logic (e.g. validation) without breaking any other code.

NOTE:

ATTENTION: The following performance considerations are only valid in VERY RARE CASES and you should NEVER ASSUME that properties have a performance penalty! If you think you could have such a case you should do performance tests using different methods (even in release mode and with optimizations a profiler could give you wrong results for such cases) to compare.

As Chris correctly mentioned, properties can be very marginally slower then members, if they are not inlined by the JIT (which should happen with auto-properties nearly all the time), because of the "method call" for the getter/setter. This is not relevant in most cases, but at MANY thousand calls per second it MAY get relevant. In such cases you should do performance tests to see if this is the case (you need to run such tests in release mode with optimizations enabled and not only use a profiler!) and only if so may use public members.
I have such a rare case in one of my programs where it got relevant somewhere between 10k-100k calls per second, but there we are still only talking about a few milliseconds for 100k calls. And even in such cases I recommend using the properties if you do not need absolutely every bit of performance you can get (as it was in my case), as the maintainabilty is more important in that case in my opinion.

edited Jun 18 '14 at 09:10

answered Jun 18 '14 at 06:28

Christoph Fink

22,727
9
68
113

4

With properties, you can also do `public string PlayerName { get; private set; }`, so *reading* is public, but *writing* is still private. – Corak Jun 18 '14 at 06:31
@Chris: Was that the reason for the down-vote or is something else wrong? Because, yes you are right. I actually also use some "public members" in some high-performance cases (~100.000 calls/s) - will add that as a note. – Christoph Fink Jun 18 '14 at 07:19
@Chris I'd be interested to see your experience proved. There's plenty of answers on SO that prove otherwise. http://stackoverflow.com/a/3265377/360211 – weston Jun 18 '14 at 08:00
-1 Because of the performance comments. I know you heavily disclaim it, but the hans-passant tests over 10M calls shows no difference. So 100k could never. If you do see a difference, you may not have a release build or disabled Jit optimisations. – weston Jun 18 '14 at 08:03
@weston: In most cases yes, but there definitely are cases where it can make a (small but existent) difference as I had one of those (possibly because I have up to 8 threads "hammering the props"). And even if not you would detect that at `In such cases you should do performance test to see if this is the case (you need to run such test in release mode with optimizations enabled!) and only if so use public members.` (which you would do anyhow in such high-performance use-cases) so I do not think my notes are wrong. But thanks for explaining your down-vote... – Christoph Fink Jun 18 '14 at 08:28
@weston - a bit harsh. "if they are not inlined". If they are, then yes, there will be absolutely no penalty whatsoever. If they are *not* there *is* a ever so slight penalty which will never ever matter in real life. Until it does (but that must be backed up by profiling!). – Corak Jun 18 '14 at 08:31
How would the JIT know you are going to hammer it with multiple threads and so not inline it? Unless you have synchronisation e.g. locks. Locks and the such will affect optimisations, as it ceases to be simple properties setters and getters. – weston Jun 18 '14 at 08:38
@weston: I don't know how the JIT "knows that" - fact is it made a (minor) difference in my case (the props where auto-props, but there are some locks around when they are accessed). We tested and profiled that part of our app heavily and I am very confident that these properties in question wher NOT inlined (even in release and with optimizations), because for these few I could measure a difference, but not for any other props. – Christoph Fink Jun 18 '14 at 08:43
@Corak well that is the heavy disclaimer I talk of. I just think of the newbie reading that and saying, "I'm going to call this 10k times, and I read on SO from a 6K rep that it sometimes matters, so I'm going to use a public field". But it's not true. Happy to see a counter example though. – weston Jun 18 '14 at 08:43
@chrfin And you profiled the release version running outside the debugger? Then that was the slowest part of the code. i.e. there was nothing else you could have optimised to save more time? – weston Jun 18 '14 at 08:44
@chrfin yes, if you have locks, then it's not a simple property, it's a whole method, it can't be as readily inlined. – weston Jun 18 '14 at 08:48
@weston: I did not only use the profile, because it changes the results sometimes for such stuff. I made several different tests to come to that conclusion, but the difference was reproducable. But you are totaly right with `I just think of the newbie reading that and saying, "I'm going to call this 10k times, and I read on SO from a 6K rep that it sometimes matters, so I'm going to use a public field"` - I will try to make it more clear that it nearly never matters... – Christoph Fink Jun 18 '14 at 08:54
I did not down vote. I believe I experienced this with simple inline properties. I had a program which iterated millions of times over data. I wanted to optimize it as much as I could and profiling lead me to remove my properties in favor of field/method variables. I had iterated many many millions of times... If you aren't, properties should be fine. – Chris Jun 18 '14 at 15:25
I should keep my mouth shut! Here is a test you can use to see how little it matters. Run that in debug and the result obviously leans to fields. Run it in release and the issue completely goes away. You should use properties for your public variables. https://bitbucket.org/apexearth/tests/src/2f79951a7597cdc8e085aadad347f849342c06f6/Tests/PropertyTests.cs?at=default – Chris Jun 20 '14 at 04:54
@Chris: Your tests are a perfect example for method inlining and I would have been surprized if the results where different. But it is not always that simple. Maybe I find the time to create an example to reproduce my case (but it's not easy as it is a complex part of a bigger project)... – Christoph Fink Jun 20 '14 at 08:08

Is it okay to make all variables public?

1 Answers1