Encapsulation, fields and properties for Asteroid class

After changing all variables in this code from public to private and adding get/set according to the need to get modified and/or read in another class, I'd like to know if this was done correctly.

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

private Rectangle boundingBox;
public Rectangle BoundingBox
{
get { return boundingBox; }
}

private Vector2 position;
public Vector2 Position
{
get { return position; }
}

private bool isVisible;
public bool IsVisible { get; set; }

private Vector2 origin;
private float rotationAngle;
private int speed;
private Texture2D texture;
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;
}

// 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 cir

Fields vs Properties

As discussed in the comments above, there's a problem with IsVisible:

private bool isVisible;
public bool IsVisible { get; set; }

When this code compiles, you'll basically get something like this "under the hood":

private bool isVisible;

private bool IsVisible;
public bool get_IsVisible() { return this.IsVisible; }
public void set_IsVisible(bool value) { this.IsVisible = value; }

BUG!
You're using isVisible all over your code, but the setter for IsVisible, set_IsVisible, is never called. Hence your clients will not see the right value... literally: the public getter will always return default(bool)... false!

You really have two options.

Either you expose data, or you encapsulate data.

Think of what a class really is: it's a definition for a type. A type can be a class, but in C# you can also have an enum, a struct, an interface... types can also be abstract (like an interface, or an abstract class).

When you instantiate a class, you create an object of the type defined by the class. An object has an interface (do not confuse with interface) that exposes everything that's publicly available. The client code doesn't "see" anything that's private or protected (or internal if the client code is in another assembly). And that's exactly where encapsulation is: it's hidden, encapsulated inside the inner workings, the implementation details of the type - the interface doesn't expose it because client code does not need to know what the implementation details are.

Fields are implementation details. They don't need to be exposed, and doing so breaks encapsulation.

Option One

You could make IsVisible an auto-property:

public bool IsVisible { get; set; }

Or, for a get-only auto-property:

public bool IsVisible { get; private set; }

Remember that under the hood, there is a private field for that auto-property (see "When this code compiles..." above).

Option Two

You could make IsVisible a property that uses a _isVisible private backing field:

private bool _isVisible;

public bool IsVisible
{
    get { return _isVisible; }
    set { _isVisible = value; }
}

The get-only version simply omits the setter.

You'll notice I'm using a naming convention that makes it a no-brainer to tell a field (_isVisible) from a parameter (value - implicit parameter in a property setter).

If you don't like having an underscore prefix on your private fields (it really comes down to personal preference.. or to the naming conventions your team is using), you could alternatively do it like this:

private bool isVisible;

public bool IsVisible
{
    get { return this.isVisible; }
    set { this.isVisible = value; }
}

The this qualifier isn't really needed (it's actually redundant in this case), but then if you had a method like this:

private void DoSomething(bool isVisible)
{
    // "isVisible" in this scope refers to the parameter.
    // use "this.isVisible" to refer to the private field.
}

And this is why I prefer the underscore prefix.

Bottom line: Properties win.

Some useful references:

Member Design Guidelines (Field Design) on MSDN:


We exclude constant and static read-only fields from this strict restriction, because such fields, almost by definition, are never required to change.

• X DO NOT provide instance fields that are public or protected. You should provide properties for accessing fields instead of making them public or protected.

• √ DO use constant fields for constants that will never change. The compiler burns the values of const fields directly into calling code. Therefore, const values can

never be changed without the risk of breaking compatibility.

• √ DO use public static readonly fields for predefined object instances. If there are predefined instances of the type, declare them as public read-only static fields of the type itself.

• X DO NOT assign instances of mutable types to readonly fields.

Note that in oop, static, predefined instance is generally just bad wording for don't do that!!

Also while you certainly don't want a mutable readonly struc, but readonly in front of a reference type only means that the reference cannot be reassigned. These are guidelines after all ;)

Member Design Guidelines (Property Design) on MSDN:

• √ DO create get-only properties if the caller should not be able to change the value of the property. Keep in mind that if the type of the property is a mutable reference type, the property value can be changed even if the property is get-only.

• X DO NOT provide set-only properties or properties with the setter having broader accessibility than the getter. For example, do not use properties with a public setter and a protected getter. If the property getter cannot be provided, implement the functionality as a method instead. Consider starting the method name with Set and follow with what you would have named t

StackExchange Code Review Q#49188, answer score: 15