Encapsulation, fields and properties for Asteroid class

Question

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 circle = MathHelper.Pi * 2;
        rotationAngle = rotationAngle % circle;            
    }
}

Answer 1

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 the property. For example, AppDomain has a method called SetCachePath instead of having a set-only property called CachePath.
• √ DO provide sensible default values for all properties, ensuring that the defaults do not result in a security hole or terribly inefficient code.
• √ DO allow properties to be set in any order even if this results in a temporary invalid state of the object. It is common for two or more properties to be interrelated to a point where some values of one property might be invalid given the values of other properties on the same object. In such cases, exceptions resulting from the invalid state should be postponed until the interrelated properties are actually used together by the object.
• √ DO preserve the previous value if a property setter throws an exception.
• X AVOID throwing exceptions from property getters.

---

Abstraction

If I look at the interface of an Asteroid, I see these members:

Asteroid(Texture2D newTexture, Vector2 newPosition);

Rectangle BoundingBox { get; }
Vector2 Position { get; }
bool IsVisible { get; set; }

void LoadContent(ContentManager content);
void Draw(SpriteBatch spriteBatch);
void Update(GameTime gameTime);

I would think pretty much everything that's drawable in your game has a very similar interface, if not an identical one.

If you defined an abstraction for the methods this interface exposes...

public interface IDrawableContent
{
    void LoadContent(ContentManager Content);
    void Draw(SpriteBatch spriteBatch);
    void Update(GameTime gameTime);
}

Then you could have every drawable game component implement that interface, like this:

public class Asteroid : IDrawableContent

And since your GameScreen (I'm making this up) also has these methods with the same signature, why not do this as well:

public class GameScreen : IDrawableContent

Your AsteroidZapperGame class (or whatever it's called) also has similar methods (defined by the XNA Game class), but it's pretty much your top-level object, so you don't need it to implement the interface.

---

What gives?

The GameScreen class might roughly look like this (over-simplified):

public class GameScreen : IDrawableContent
{
    // _items is readonly. 
    // its reference may only be assigned in a constructor.
    // items may still be added or removed from the list any time.
    private readonly IList<IDrawableContent> _items;

    public GameScreen(IList<IDrawableContent> items)
    {
        _items = items;
    }

    public void LoadContent(ContentManager content)
    {
        foreach (var item in _items)
        {
            item.LoadContent(content);
        }
    }

    public void Draw(SpriteBatch spriteBatch)
    {
        foreach (var item in _items)
        {
            item.Draw(spriteBatch);
        }
    }

    public void Update(GameTime gameTime)
    {
        foreach (var item in _items)
        {
            item.Update(gameTime);
        }
    }
}

This class has no clue that you're making it draw asteroids. In fact, it could just as well be drawing bubbles in a soda can, or a bunch of zombies hunting the player down. That's what abstractions do: they abstract away the implementation details, and that makes code easier to read, extend, and maintain.

Answer 2

This is a slippery slope here

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);
}

Especially when in the next method you write the if statement like this

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

in the first example it is unclear whether the statement is actually enclosed inside of the if statement because there is a commented out line of code there, I know it will compile, but you should do one of these things (or two)

• Delete the comment (write yourself notes somewhere other than in the code, so you know what you changed)
• Don't one line the isVisible if statement
• One line all of your if statements

---

I would write it like this personally

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

This is very clear what your intent is and doesn't clutter with Comments that hide code that could confuse you when you maintain the code later.

Always be consistent in the way that you code if statements and other blocks of code, because it will make it much easier to maintain later.

---

---

you probably should look at disposing the asteroid once it goes off-screen rather than holding the variable while it is not visible, it will make it easier for when you want to expand the game to include more objects.

right now I have an issue of too many shots from my ship not being disposed fast enough, but that is because I created a world map where I use a viewport to travel the world and I wanted my shots to kill things off screen (still a lot of things that need to be changed like viewport size and stuff lol)