Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have read several articles using Google, but I somehow do not really get what they do, and when it is needed. Below I'll give 2 pieces of code, in one using get/set and in the other without, and they both work (at least in this example).

Example 1

public class Example
{
    public int number;
}

class Program
{
    static void Main(string[] args)
    {
        Example example = new Example();

        example.number = 5;

        Console.WriteLine(example.number);
        Console.ReadKey();
    }
}

Example 2

public class Example
{
    private int number;
    public int Number
    {
        get { return number; }
        set { number = value; }
    }
}

class Program
{
    static void Main(string[] args)
    {
        Example example = new Example();

        example.Number = 5;

        Console.WriteLine(example.Number);
        Console.ReadKey();
    }
}

I've added a piece of real code below so it may be improved and perhaps shown examples on these accessors.

// Main
public class Asteroid
{
    Random random = new Random();

    public Texture2D texture;
    public Vector2 position, origin;
    public float rotationAngle;
    public int speed;
    public Rectangle boundingBox;
    public bool isVisible;
    public float randomX, randomY;

    // Constructor
    public Asteroid(Texture2D newTexture, Vector2 newPosition)
    {
        position = newPosition;
        texture = newTexture;
        speed = 4;
        randomX = random.Next(0, 750);
        randomY = random.Next(-600, -50);
        isVisible = true;
    }

    // Load Content
    public void LoadContent(ContentManager Content)
    {

    }

    // Draw
    public void Draw(SpriteBatch spriteBatch)
    {
        if (isVisible)
            // spriteBatch.Draw(texture, position, Color.White);
            spriteBatch.Draw(texture, position, null, Color.White, rotationAngle, origin, 1.0f, SpriteEffects.None, 0f);
    }

    // Update
    public void Update(GameTime gameTime)
    {
        // Set bounding box for collision
        boundingBox = new Rectangle((int)position.X - (texture.Width / 2), (int)position.Y - (texture.Height / 2), texture.Width, texture.Height);

        origin.X = texture.Width / 2;
        origin.Y = texture.Height / 2;

        // Update Movement
        position.Y = position.Y + speed;

        if (position.Y >= 950)
        {
            isVisible = false;
        }

        // Rotating Asteroid
        float elapsed = (float)gameTime.ElapsedGameTime.TotalSeconds;
        rotationAngle += elapsed;
        float circle = MathHelper.Pi * 2;
        rotationAngle = rotationAngle % circle;            
    }
}
share|improve this question
    
This question appears to be off-topic because it is about seeking understanding of code, and not seeking a review. –  Jamal May 7 at 20:09
2  
This isn't the right site for this. But basically, the first is always wrong. It allows people to freely edit the stuff that makes your class work. The second allows you to control access rights and set up notifications on changes. Additionally, changing from the first to the second requires recompiling all code that uses your dll. If you have to make something public, do the second. –  Magus May 7 at 20:09
    
@Jamal I'm trying to get a review between those 2 things, as I've been using the first method for a lot of stuff and I feel like it is wrong, as in a lot of examples I see the use of get and set. –  anthonytimmers May 7 at 20:11
1  
The difference is that the first exposes a public field, the second encapsulates a private field with a public property. There's also option 3: public int Number { get; set; }, an auto-property (C# 3.0+) which is exactly like option 2. Don't expose public fields. –  Mat's Mug May 7 at 20:16
2  
Accessors are always better if something needs to be public. However, you shouldn't make anything public unless it must be. When you do, read only ({get;private set;})is preferable when possible. There really are no advantages to a public field. –  Magus May 7 at 20:29
show 11 more comments

4 Answers 4

up vote 2 down vote accepted

First of all, I'd make the texture and position fields private. They're set from the outside, and the outside doesn't need to change them.

Then I'd extend that. Anything your class needs to be given, add to the constructor. If Asteroid makes it itself, make it private. If something needs to look at it, make a get only property to access it. If it needs to be modified, make it an autoproperty.

The only thing that seems like a good candidate for a property to me is your Origin. Something may want to sort Asteroids by Origin.

If, following my advice, you reach more than 4 parameters, you should make a new class to either use as the parameter or deal with those values in the Asteroid.

If your teachers have been telling you to make everything a public field, ignore them. They're wrong. Maybe follow their advice on assignments for the sake of credits, but never in production code.

share|improve this answer
    
So to see if I get what your saying correctly.. The asteroids position needs to get read to determine where to spawn a explosion when it gets shot, so this needs to have a get only propery? –  anthonytimmers May 7 at 21:04
    
Yes. The fact that something else needs to know where something is is understandable. The idea that any arbitrary thing can modify it is not. That's the basic premise behind my answer and comments. –  Magus May 7 at 21:19
    
I have changed everything within Asteroid.cs and no errors and it still functions. However I'd like to show it now and see if what I did was correct.. Posting it as a comment however completely ruins any lay-out :-(, any way how I could show the code? Just add it to the main post? –  anthonytimmers May 7 at 21:22
    
Yes, I'd edit the main post, maybe after a horizontal divider, and explain that this is your edited code. –  Magus May 7 at 21:24
1  
show 3 more comments

Example 2 could also be written like this using autoproperties

public int Number { get; set; }

It would be good to recognize that you can set access modifiers on these

private int number;
public int Number
{
    get { return number; }
    private set { number = value; }
}

which could also be written as

public int Number { get; private set; }

In your specific example you will use getters and setters simply when something is public instead of private. You can however have private properties with getters and setters. Getter and Setters are methods which are used in the setting and getting of data. They are very useful when you need to do some calculation when getting or setting data. Example: getting calculation

DateTime Birthday { get; set;}
TimeSpan Age { get { return DateTime.Now Birthday; } }

Example: MVVM OnProperty Changed, notifies the UI when something in the backend has changed and updates accordingly

public string ContactName
{
    get { return _contactName; }
    set
    {
        if (_contactName == value) return;
        _contactName = value;
        OnPropertyChanged("ContactName");
    }
}

As I said above, C# convention is to make anything that is publicly accessible to be a property. So this is your chance to look at your class members and decide what really needs to be seen and/or modified by outside sources.

What if you decided to allow addons to your game, and lets say its a multiplayer game, so you don't want people cheating. But now anyone who can get the memory address of your asteroid object could easily change its position, visibility, or its texture. Even if this is some small project for yourself, data should only be exposed as much as absolutely necessary, and only those who need to change that data should be able to. Also, anything that is public needs to be PascalCase, not camelCase ie. start with a capital letter.

From a quick look... this is how I would do it.

public Texture2D Texture {get; private set;}
public Vector2 Origin {get; private set;}
public float RotationAngle {get; private set;}
public Rectangle BoundingBox {get; private set;}
public bool Visible {get; private set;}

private Random _random = new Random();
private Vector2 _position;
private int _speed;
private float _randomX, _randomY;

Edit:

It is important to note: that depending on the datatype making a private set does not stop the object from being modified. You could still be pointing to the same object as before, but some malicious code could have changed properties of that code. In this case, you could have the public get get a cloned version of a privately backed object.

private Person _customer;
public Person Customer {get { return _customer.Clone(); }}

Now the only customer that the public gets to toy with is a customer they can modify, but it wont affect the customer you have though.

share|improve this answer
2  
+1 especially for the last paragraph. Clear, applicable justification for a concept beginners often find obscure. –  Magus May 7 at 21:23
1  
Last paragraph really helped me understand why it is important not to leave fields public. –  anthonytimmers May 7 at 21:24
    
Anyone who has the memory address of your variables will use memory editing software which bypasses all and any language-level private modifiers. Please don't treat private modifiers as offering any security. –  Lie Ryan May 8 at 1:25
    
@LieRyan I'm talking about pointers. If you can get a reference to an object you shouldn't, you would be allowed to change it to you're hearts content. Imagine that you are connecting to an online service, you cant use your memory hacking software to find and change a value on the server, BUT if they leave it public, then yes you can. –  BenVlodgi May 8 at 11:54
add comment

I'd probably go one farther - if this class has an immutable state in that number (I don't know the use case for your actual class), make it immutable as such:

public sealed class Example
{
    private readonly int number;
    public Example(int number)
    {
        this.number = number;
    }
    public int Number
    {
        get { return this.number; }
    }
}

class Program
{
    static void Main(string[] args)
    {
        Example example = new Example(5);

        Console.WriteLine(example.Number);
        Console.ReadKey();
    }
}

If it is a simple value that can change, make it mutable and add the property setter. If there is some other action that happens when the number changes, make a method SetNumberForPinballInitialization(int number) or some such.

share|improve this answer
add comment

The answer to this is more subtle than most would admit. Whether or not to use getter/setter is highly dependant on the features of the language you're using and most importantly the intent of the class.

In languages like Java and C++, exposing public fields is considered bad practice, because it allows external code to modify internal mechanism of the object and invalidate the object's contracts and invariants. In these languages, getter and setter are an unavoidable way of life, as their syntaxes limits their possible options.

In languages like Python, exposing attributes as public is considered OK, because attribute access in Python is actually a method call to the attribute's descriptor; in simpler words, in these languages, you can easily override an attribute assignments to maintain the class' contracts and invariants. In languages that allows transparent transition between attributes and properties like Python, it is common that the language doesn't even need/have private attributes.

Like Python, C# allows overriding attribute assignment; unlike Python, in C#, changing attribute to property and vice versa breaks binary compatibility. Thus, if you ever wanted to make the attribute public in C#, you should do it as autoproperty instead of as public attribute. There are also more subtle differences between changing attributes to properties, so starting with property is generally a good idea.

Immutability is a different beast. Immutable classes are easier to reason with than mutable classes and can be safely passed around without the need for copying so immutable classes are generally very desirable. In many softwares, it is a good idea to have a certain set of classes (i.e. value-type classes) to be immutable. Value-type classes are essentially a bundle of data that needs to go together with zero behaviour in itself, in other words, a struct. Examples of value-type classes are classes that represents Numbers, Vectors/Coordinates, Matrices, Colours, etc. Strictly speaking, there is really not much of a real issue in exposing publicly attributes of a bundle of data with zero behaviours. For value-type objects, most people advocates the use of autoproperties for consistency and a "just-in-case" attitude, and because properties are very cheap anyway; while some people advocates to just do away with properties under this very limited circumstances. I'd say, if you're ever in doubt, just use property.

Having said that. Most non-value-type classes should never expose attributes whether it's as properties, getter/setters, or public attributes. Classes that are not value-type classes should expose behaviours rather than attributes and it is these behaviours that you should emphasize. Instead of public int X { get; set } and public int Y { get; set }, expose .Move(coordinate); instead of .GetImage(), expose .Draw(canvas); etc. If you have lots of accessors for every fields in the class, then you're most likely doing something wrong, you're not encapsulating the data.

In summary, you are asking the wrong question. Your question should not be whether to use getter/setter vs attributes nor should it be getter/setter vs property. Your question should be whether these accessors should exist in the first place; more often than not, having accessors produce anemic domain model anti-patterns.

This is how I might do the Asteroid class, no accessors should be needed as Asteroid should be a class with rich behaviors:

public class Asteroid
{
    private Texture2D texture;
    private Vector2 position, origin;
    private float rotationAngle;
    private int speed;
    private Rectangle boundingBox;
    private bool isVisible;
    private float randomX, randomY;

    // Constructor
    public Asteroid(Texture2D newTexture, Vector2 newPosition)
    {
        position = newPosition;
        texture = newTexture;
        speed = 4;
        randomX = random.Next(0, 750);
        randomY = random.Next(-600, -50);
        isVisible = true;
    }

    public void Show(bool show=true) { ... }
    public void Hide() { ... }

    public void Draw( ... ) { ... }

    public void Update( ... ) { ... }    
    public void CollidesWith( Collideable ) { ... }


}
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.