Is this bad OO programming? Passing this down through methods

Question

Ok, assume I have a class, X and X is something which has an aggregate relationship with other objects. Lets pretend X is a soccer stadium.

X is full of class spectators. However, the behaviour of each spectator for a particular activity differs. Instead of IF statements, I want the different behaviour to be within the spectator class, so that I can use dynamic binding.

However, the problem is that the behaviour the spectator performs affects the "soccer stadium" class. So I was thinking of passing "this" from the soccer stadium class, through a method, to the Spectator class, so that the spectator class can do something to the Soccer Stadium class?

public class SoccerStadium{
    SpecatorInterface s = new Spectator();

    public void SpectatorBehaviour(){
        s.doSomething(this);
    }

    public void doSomethingthingBySpecator(){
    }
}

public class Spectator implements SpecatorInterface{
    public void doSomething(SoccerStadium s){
        s.doSomethingthingBySpecator();
    }
}

I only want to do this so that I can use dynamic binding and alter the behaviour in Specator.doSomething() so that I can have lots of different types of SpectatorSuperClass as an attribute passed to SoccerStadium and then have the different behaviour.

EDIT: What if I passed the reference of the Stadium to the Specator through the Spectator constructor, instead of passing this?

Answer 1

This isn't so much "bad oo programming" as it is tightly coupled. There's nothing inherently wrong with passing around this pointers, but it can become a mess very very quickly. We can't really say more without more information.

Answer 2

I see no problem with the usage of this as a parameter. Nevertheless, I don't like the new Spectator() call that was hard coded in your SoccerStadium class. I believe you should have a Factory with a createSpectator method that could receive a parameter indicating which type of spectator you intend to create.

Answer 3

To me, this kind of two-way circular relationship is bad news. What if Spectators want to go to the Theatre instead?

I'd decouple the relationship by making the Stadium a subscriber to Spectator dispatched events.

public class SoccerStadium
{
    ISpectator s = new Spectator();
    public SoccerStadium()
    {
        s.DidSomething+=DoSomethingthingBySpecator;
    }
    public void SpectatorBehaviour()
    {
        s.DoSomething();
    }
    public void DoSomethingthingBySpecator(object sender,EventArgs e)
    {
        Console.WriteLine("spectator did something");
    }
}
public interface ISpectator
{
    event EventHandler DidSomething;
    void DoSomething();
}
public class Spectator:ISpectator
{
    public event EventHandler DidSomething;
    public void DoSomething()
    {
        var ev=DidSomething;
        if(ev!=null)
        {
            ev(this,EventArgs.Empty);
        }
    }
}

...and so the Spectator now has a means of communicating to anything that's interested, but doesn't need to know a thing about it.

Answer 4

As people have said, there's absolutely nothing wrong tight tight coupling and what you are doing. However, if you want a little bit of decoupling, use the classic visitor pattern.

public interface SpectatorVisitor {
  ...
  void visit(Spectator spectator);
}

public class Spectator {
  ...
  public void accept(SpectatorVisitor visitor) {
      visitor.visit(this);
  }
}

public class Stadium {

  ...
  spectator.accept(new StadiumSpectatorVisitor());
}

The visit method signature could be altered to accept some kind of state object as well if you need to. Otherwise you could simply define the relevant methods on the Spectator class, and make the visitor collect up the information needed to alter the stadium.

For instance:

public class Spectator {
  private Team supports;

  public Team getSupports() {
      return supports;
  }

  public void accept(SpectatorVisitor visitor) {
      visitor.visit(this);
  }
}

public class SupportedTeamVisitor {
  private Map<Team, AtomicLong> supportCount = new HashMap<Team, AtomicLong>();

  public void visit(Spectator spectator) {
     Team supports = spectator.getSupports();
     if (! supportCount.contains(supports)) {
       supportCount.put(team, new AtomicLong(0));
     }
     supports.get(team).incrementAndGet();
  }

  public Map<Team, AtomicLong> getSupportCount() {
     return supportCount;
  }
}


public class Stadium {

  public long getSupportCount(Team team) {
     SupportTeamVisitor visitor = new SupportedTeamVisitor();
     for (Spectator spectator : spectators) {
        spectator.accept(visitor);
     }
     AtomicLong count = visitor.getSupportCount().get(team);
     return (count == null) ? 0 : count.get();
  }
}

Make sense?