5
\$\begingroup\$

I think everyone has some code they are embarrassed and not proud of and today I have decided to show mine. I'm not sure how to go about making this more efficient. At the time I was just happy it did what I wanted it to do and it's the first game I ever made. So basically, this is a collision detection method for my game and it describes which objects should collide and if they do, what should happen?

Any ideas on how to improve would be much appreciated :)

void collisionHandling(GameObject other) {
    //the following method details the outcome whenever one class comes into contact with another
    if ((this instanceof Heart && other instanceof Ship) && this.overlap(other)) {
        //increments ship lives if heart is hit by ship unless the ship has 5 lives
        Game.ship.dead = false;
        if (Ship.lives < 5) Game.ship.incLives();
        this.hit();
    }
    if ((this instanceof ShieldSprite && other instanceof Ship) && this.overlap(other)) {
        //makes the ship invulnerable if the sprite is hit and sets a counter till it runs out
        Game.ship.dead = false;
        Ship.invul = true;
        Game.shipInvulCounter = 400;
        this.hit();
    }
    if ((this instanceof Enemy && other instanceof Bullet) && this.overlap(other)) {
        //if the enemy gets it by a bullet it gets hit
        this.hit();
    }
    if ((this instanceof Enemy && other instanceof Ship) && this.overlap(other)) {
        //if the enemy gets it by a bullet it gets hit
        this.hit();
        other.hit();
    }
    if((this instanceof Enemy && other instanceof BigBullet) && this.overlap(other)){
        //if the enemy gets it by a big bullet it dies instantly
        this.dead = true;
        other.hit();
    }
    if((this instanceof BigBullet && other instanceof Asteroid) && this.overlap(other)){
        //if a big bullet hits an asteroid they both get hit
        other.hit();
        this.hit();
    }
    if((this instanceof Bullet && other instanceof Asteroid) && this.overlap(other)){
        //if a bullet hits an asteroid they both get hit
        other.hit();
        this.hit();
    }
    if((this instanceof EnemBigBullet && other instanceof Ship) && this.overlap(other)){
        //if an enemy big bullet hits the ship it dies instantly
        other.dead = true;
        this.hit();
    }
    //the following 'if' statements are so that if those two objects meet, they will simply glide over eachother
    if (!Ship.invul) { //if the ship is invulnerable everything glides over it
        if (!((this instanceof EnemBullet && other instanceof Asteroid) || (this instanceof Asteroid && other instanceof EnemBullet))) { //enemy bullets should not kill asteroids
            if (!(this instanceof Enemy || other instanceof Enemy)) { //enemies do not come into contact with anything other than what is mentioned above
                if(!(this instanceof EnemBigBullet || other instanceof EnemBigBullet)) {//enemy big bullets do not come into contact with anything other than what is mentioned above
                    if (!((this instanceof Bullet && other instanceof Ship) || (this instanceof Ship && other instanceof Bullet))) { //stops the bullets killing the ship when they spawn
                        if (!((this instanceof BigBullet && other instanceof Ship) || (this instanceof Ship && other instanceof BigBullet))) { //likewise with big bullets
                            if (!(this instanceof ShieldSprite || other instanceof ShieldSprite)) { //nothing collides with shield sprite except ship as mentioned before
                                if (!(this instanceof Heart || other instanceof Heart)) { //likewise for heart
                                    if (this.getClass() != other.getClass() && this.overlap(other)) { //otherwise if the classes are different then they will hit
                                        this.hit();
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}
\$\endgroup\$
  • 5
    \$\begingroup\$ Oh boy! Did you ever hear of interfaces? \$\endgroup\$ – πάντα ῥεῖ May 11 '17 at 16:48
  • 1
    \$\begingroup\$ @πάνταῥεῖ I shudder at the thought \$\endgroup\$ – SJJ May 11 '17 at 16:51
  • \$\begingroup\$ Why actually? Those classes should decide themselves what happens when colliding with other objects. Is that implementation made within the GameObject class? \$\endgroup\$ – πάντα ῥεῖ May 11 '17 at 16:53
  • 2
    \$\begingroup\$ This method shouldn't be implemented there as mentioned, but GameObject should be abstract, and the collision detection should be left to the concrete implementaations. That default behavior that both objects are hit() could go to some base class implementation though. Lookup the Single Responsibility Principle. \$\endgroup\$ – πάντα ῥεῖ May 11 '17 at 17:02
  • 1
    \$\begingroup\$ @πάνταῥεῖ so just keep the class abstract and move the collision handling to the individual game object classes? \$\endgroup\$ – SJJ May 11 '17 at 17:05
2
\$\begingroup\$

I think everyone has some code they are embarrassed and not proud of and today I have decided to show mine.

A pattern to get out of this mess ..

That's an impressive amount of nested if-statements. As you can see, the behavior of collision handling depends both on the current instance's type and the provided instance's type. There is a pattern suited for this kind of complexity: the Visitor Pattern.

GameObject

You first declare a base method in GameObject that redirects the generic collision handling to a specific method based on the type of other.

void collisionHandling (GameObject other) {
    if (this.overlap(other)) {
        if ((other instanceof Ship)) {
            collisionHandling((Ship)other);
        } else if ((other instanceof Bullet)) {
            collisionHandling((Bullet)other);
        } 
        // and so on ..
    }
}

Derived Classes

Class Heart could then override any such method.

@Override
void collisionHandling (Ship ship) {
    Game.ship.dead = false;
    if (Ship.lives < 5) Game.ship.incLives();
    this.hit();
}

Class ShieldSprite would override that method with different behavior.

@Override
void collisionHandling (Ship ship) {
    Game.ship.dead = false;
    Ship.invul = true;
    Game.shipInvulCounter = 400;
    this.hit();
}

By implementing this pattern, the cyclomatic-complexity of collision handling method gets reduced drastically and all logic sits at the right place, which makes the design object-oriented.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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