Object A has a list of object B, how appropriate for object B to change class A?

advertisements

I've done my best to research this issue on my own, but I've been unable to find a good answer. Most likely because I don't know the proper way to ask the question. So I'm just going to throw everything out there.

This is the basic structure of the two classes I have. (Obviously not actual code.)

class ship
{
    public int baseStat = 0;    //I want this to be protected
    public int derivedStat = 0; //This should also be protected
    public int DerivedStat{ get { return derivedStat ; } }

    private List<Component> components;

    public void DeriveStats()
    {
        foreach(Component component in components)
        {
            component.DeriveStats(this);
        }
    }
}

class Component
{
    public void DeriveStats(Ship ship)
    {
        ship.derivedStat = ship.baseStat + 5;
    }
}

This all works just fine, I have many different types of Component subclasses that do different things to the ship and it's stats, but with this setup, all stats are viewable and modifiable outside the ship class. Design wise, this doesn't seem correct, and frankly, it's driving me bonkers.

Is there a cleaner/more correct way I can approach this?

EDIT-

Big thank you to everyone who commented and gave an answer, it was a huge help. In case anyone else stumbles upon this question, here's how I went about addressing the issue.

abstract class Ship
{
    //Locking variable that only allows the ship's stats to be updated when true
    private bool _isModifying = false;

    //General Stats that define a ship******************************

    //Layers of shields
    protected int baseShields;
    public int DerivedShields
    {
        get{ return DerivedShields; }
        set
        {
            if (_isModifying)
                DerivedShields = value;
            else
                throw (new Exception("Ship stats cannot be modified outside the ship class."));
        }
    }

    //**************************************************************

    protected List<Component> installedComponents;

    public void DeriveStats()
    {
        _isModifying = true;

        foreach (Component component in installedComponents)
        {
            component.DeriveStats(this);
        }

        _isModifying = false;
    }
}

class BasicShield : Component
{
    public override void DeriveStats(Ship ship)
    {
        ship.DerivedShields = ship.DerivedShields + 1;
    }
}


Add methods to Ship that will allow others to alter Ship but gives Ship the responsibility for and ability to ensure its own consistency. This means that

  1. external parties can request that Ship be updated, but Ship can deny such requests if they would violate some constraint (or if Ship just doesn't feel like being updated for whatever reason).
  2. you can give these methods names that convey the semantics of the operation, while you decouple the implementation details of Ship from the rest of your program. You gain the freedom to change the way you represent a Ships internal state.

EDIT:

If you want each Component to implement the algorithm that determines the new stats, you could

  1. Make the ship's properties be read-only (they will have to be visible for the component to make its calculations)
  2. Make the component's DeriveStats(...) method return the calculated value
  3. Make the ship itself call DeriveStats(...) and update its values accordingly.

This has the added benefit that the ship is in charge of aggregating results from different components. As the components don't know about each other, how would they ever coordinate who gets to decide the ship's stats?