Best practice - public fields or private with access methods?

Is it best to use public fields in classes that you intend to access, or set/get methods?

eg. Defining an engine class

    using UnityEngine;
    using System.Collections;

    public class Engine : MonoBehaviour {
        public float max_speed;
        public float turn_rate;
    }

…and accessing its values in another script…

    Engine engine = gameObject.GetComponent<engine>();
    speed = engine.max_speed;
    engine.turn_rate = 10.0f

OR

    using UnityEngine;
    using System.Collections;

    public class Engine : MonoBehaviour {
        float max_speed;
        float turn_rate;

        public float get_speed(){
            return max_speed;
        }

        public float set_speed(float s){
            max_speed = s;
        }

        public float get_turn_rate(){
            return turn_rate;
        }

        public float set_turn_rate(float t){
            turn_rate = t;
        }
    }
    Engine engine = gameObject.GetComponent<engine>();
    speed = engine.get_speed();
    engine.set_turn_rate(10.0f);

The latter seems more proper programming form, but adds an awful lot of faff IMO. My main concern is performance, and then readability.

What are you guys using, and why?

Are you new to C#? Because you’re writing explicit method that take the role of properties.

public float Speed
{
    get { return speed; }
    set { speed = value; }
}

However, out of the box, Unity only expose public fields in the Inspector, or private fields flagged with the SerializedField attribute.

There’s a few plugin around that allow you to add properties to the exposition in the Inspector.

As for me, I built my own framework and I only expose properties.

Yes, I’m C# untrained and just winging it with prior C++ and Java knowledge. :wink:

Until you are actually doing something in the Get/Set you can just use auto-properties which will create an anonymous private var for you in the background. Reduces clutter.

public string Name { get; set; }
1 Like

But why if it’s performing no additional tasks? It is just overhead for no reason.

Validation you would normally do in your traditional get and set functions can be built into C# properties. I think this makes the code look much cleaner than Java/C++. You can also have a public getter with a private setter.

private int m_health;
public int health {
  get { return m_health; }
  private set { m_health = Mathf.Clamp(value, 0, maxHealth); }
}

The overhead is VERY small. Talking here of fraction of nanoseconds. By the way, you may not know it, but fields also have hidden accessors.

I add a property because it gives me control on how the variable are changed by the user. And even if I only had a “bare-bone” property, it still gives me a lot more control on how the stuff is displayed on my own Inspector.

Frankly, exposing only fields is arguably a design flaw. Unity is the only engine - that I know of - that expose fields without any kind of security layer.

[quote=“LightStriker_1, post:7, topic: 524087, username:LightStriker_1”]
The overhead is VERY small. Talking here of fraction of nanoseconds. By the way, you may not know it, but fields also have hidden accessors.
[/quote]On this page about singletons, they do a test where they store the singleton instance in a C# property and again with a C# field. The field is faster. But not by much!

[quote]
We are looking at tiny amounts of time here. The times for the benchmark of 1 billion iterations was 1.935 seconds versus 1.731 seconds.
[/quote]That comes to 0.204 NANOseconds lost from using a property instead of a field. So on that test computer, we would have to access the property 4901961 times before we even saw a difference of 1 millisecond in the profiler.

That page I linked does suggest using fields, but we’re making games, whose code is always changing, and trying to do it fast to market. Removing a single draw call probably has a million times (no exaggeration!) more effect, compared to worrying about using a field instead of a properly.

[quote=“LightStriker_1, post:7, topic: 524087, username:LightStriker_1”]
Frankly, exposing only fields is arguably a design flaw. Unity is the only engine - that I know of - that expose fields without any kind of security layer.
[/quote]Yeah, this was something I hated for a long time. Stuff was public so we could assign in the inspector, but that also meant that every other class had access to the field!

At my workplace, we prefix all fields with m_… So if there’s a public field starting with m_… it is verboten to access it! If we want it to truly be publicly accessible, we make a public property without the m_ prefix.

// only public so we can assign in the inspector!
// Anyone caught using this field from another class will be humiliated publicly!
// (All good-natured fun =p)
public Transform m_someTransform;

// This one is okay to use.
public Transform someTransform {
  get { return m_someTransform; }
}

So while this still just offends some part of my coder soul somewhere, at least we know by convention what is supposed to be public, and what is only public because Unity forces us to make it public.

Why not:

[SerializeField]
private type name;

Now you can assign in inspector but it’s private

I guess I’m a bit more protective of my coder soul. :stuck_out_tongue:

I rewrote the Inspector so it would take Properties, and about a dozen other features I still don’t understand why they are not stock in Unity.

While it’s “better” from a security point of view, it’s still not enough. Let me give you an example… I wrote a path/spline class that support vector projection. However, for that to work properly, I have to sub-divide the spline into smaller chunks (for math). That data is serialized, so I don’t need to recalculate it when the object is loaded.

To be sure the data is correct, I have to do that anytime someone move a node or a handle around. If I had exposed it as fields, I would had no way to trap the moment one of those values are changed.

I admit spaghetti code does sometimes cause issues, but when working in a one-man team, what exactly is the security risk with having public fields? If your code is able to change a field value anywhere by using get/set, what’s the difference between accessing it directly via it being public? I don’t know how long you’ve been coding, but back in the old days before fancy structured languages, public variables were a great way to ease code development and keep things speedy. Heck, you’d even forgo variables and just use pointers everywhere! :slight_smile: I wouldn’t recommend it at all for complex systems or critical system or even development teams, but it had its uses. I’d like to learn why ‘private var with get/set’ is better than ‘public var’. If it’s just a matter of system robustness and security between classes, in a one-man team built game those don’t particularly important.

The post just above your, I gave an example of why I sometime must have a property. Performing an action when something change is sometimes quite important.

It’s also (sometimes) useful to expose a different type than what is used internally.

Monodevelop also has a useful shortcut for adding the public get/set for a field. If your field is private and starts with a lowercase letter, you can do alt+insert to bring up a code generation prompt, and select the properties you’d like to generate getters and setters for. There’s also a read only option. Saves a lot of time compared to typing it out.

I’ve just encountered something I don’t understand regards XMLSerializer. In order to export my object designs to an xml savefile, I appear to need the fields to be public. If there’s no way around this, aren’t I somewhat forced into using public variables anyway? Or would people recommend something like private fields for the game code and a public class only for the reading + writing of data?

Ah yeah… Isn’t the XMLSerializer different from any other C# serializer? I’m pretty sure it is. Frankly, serialization is always up to you. You can use the serializer directly as it is, or you can always use Reflection to force it to do what you want.

But first… Why do you need to serialize XML files?

This solution I think meats all your criteria:
http://wiki.unity3d.com/index.php?title=Expose_properties_in_inspector

LightStriker, I don’t think serializing the subdivisions of splines is a good idea, because serialization might be more expensive than just recalculating that after loading, or even better maybe recalculate them on the fly and save memory. IMHO you’ve chosen the worst option out of these three ;)… External storage is way slower than the CPU, and spline calculations are much simpler than the code involved in serialization. Third option might be the best as that would minimize working memory usage, resulting with better data locality that may further improve performance because you’ll end up with higher chances of accessing data from cache memory instead of much slower RAM. And yes, don’t take these claims as true unless you verify them by profiling :stuck_out_tongue:

It’s the basis I used for my own framework. Now it’s way bigger… 3-3.5k lines just for the Inspector.

We load everything upfront and do instances of them, so the external memory being slower is not really an issue. Seriously, talking about 1-5 ms per spline of recalcul, depending on the subdivision. For an iOS game, when we instanciate 4-5 of those at once, it’s not funny for the framerate. Better serializing a few Vector3. We did test it.

For PC I wouldn’t care about saving that data and I would let the CPU handle it.

And in the above example, you also have to recalculate the spline in the Editor as the user edit them. If a scene has a hundred of them, we can’t “just” recalculate everything all the time, just in case something change.

Here’s another example, clamping values;

public float TransitionTime
{
    get { return transitionTime; }
    set { transitionTime = Mathf.Clamp(value, 0, float.MaxValue); }
}

or another, verifying that a localizable texture is readable;

public Texture2D Temporary
{
    get { return temporary; }
    set
    {
        if (temporary != value)
        {
            if (value != null)
            {
                try
                {
                    value.GetPixel(0, 0);
                    temporary = value;
                }
                catch (UnityException)
                {
                    Debug.LogError("Localized Texture must be readable.");
                }
            }
            else
                temporary = value;
        }
    }
}

Exposing elements of a fixed sized array?

public CameraPositionner PositionnerB
{
    get { return positionners[1]; }
    set { positionners[1] = value; }
}