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?

Answer 1

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.