Abstraction Question

[Emma Sophia Jones]

Hey guys,

I have a quick question with (hopefully) a fairly simple response. I'm cobbling together a Space Invaders game and have found myself wanting to create three different types of object. The base class for these objects would be something like "Entity" which would have three sub-classes:

- MovingEntity
- GraphicEntity
- MovingGraphicEntity

The thing is, that last one doesn't sit right with me. Even if it extends "MovingEntity" it's going to result in me repeating the code from GraphicEntity. In an ideal world, the object would just extend both MovingEntity and GraphicEntity but I know that that isn't possible in Java...

So how do I keep my code dry? I'm assuming the answer will be something to do with interfaces, but I've never put one of those together and immediately ran into hurdles when I tried to create one ("Entity" has an x value, but the "MovingEntity" interface couldn't adjust that X value or get access to the getters/setters).

Thanks in advance!

[Stephan van Hulst]

When you want to reuse pieces of behaviour in different classes that are part of the same inheritance tree, you can use traits. In Java, you can simulate traits with interfaces and default methods:

interface Movable {

  Location getLocation();

  void setLocation(Location location);

  default void move() {
    // do something with getLocation() and setLocation().
  }
}

interface Graphic {

  Location getLocation();

  default void display() {
    // do something with getLocation().
  }
}

final class MovingGraphicEntity extends Entity implements Movable, Graphic {
  //
}

[Emma Sophia Jones]

Okay that makes sense...

So then if I want the Movable interface to give the Entity object an xSpeed variable, is there a way for me to keep that variable private so that the client can't access it directly? Or do I have to set xSpeed as a public double within the Interface definition?

public interface Movable {
	
	double getX();
	void setX(double x);
	
	default void move(long delta){
		
		//delta is the time since the last loop was run
		double pixelsToMove = ((xSpeed * delta) / 1000);
		setX(getX() + pixelsToMove);
		
	}
	
	public double xSpeed = 0; //Can everyone see me?
}

[Swastik Dey]

Variables declared inside an interface by default coderanch, static and final, and you won't be able use any specifier other than these.

[Emma Sophia Jones]

So I'm basically forced to make a call between good practices for information hiding or good practices for abstraction? No way to have both in this case?

[Stephan van Hulst]

You can't use instance fields with an interface, and properties declared on an interface have to be coderanch. If that's not what you want, then you'll have to abandon inheritance and go for composition and delegation instead:

final class MovingGraphicEntity implements Movable, Graphic {

  private final MovableImplementation movable;
  private final GraphicImplementation graphic;

  double getX() {
    return movable.getX();
  }

  double getHorizontalSpeed() {
    return movable.getHorizontalSpeed();
  }

  @Override
  public void move(long delta) {
    movable.move(delta);
  }

  @Override
  public void display() {
    graphic.display();
  }
}

[Emma Sophia Jones]

Okay.

Final question, I think, on this. When would you use inheritance and when would you opt to go for composition/delegation instead?

[Junilu Lacar]

I smell a design approach smell.  It looks like you're focusing on attributes first. This leads to all kinds of bad design decisions. Focus on behavior and relationships/collaborations first. That will lead you to better decisions as to how to use/reuse code and which class is the most logical one to make responsible for managing data/information.

[Stephan van Hulst]

I almost never extend classes. I find that almost all my programs consist of interfaces and final classes that implement those interfaces. If I do extend a class, it's an abstract class that partly implements an interface using behaviour that should be the same for all classes that implement that interface.

As Junilu pointed out, your problem might be caused by bad design. For instance, it's not clear to me why an object should be both graphic and movable. That sounds as if your class is part of both your business and your presentation layer, which should be separate concerns.

[Junilu Lacar]

I'd try to avoid defining default interface methods with new interfaces.  Default interface methods were introduced into the language as a mechanism to support backwards compatibility. Adding a default method to an existing interface makes sense because it mitigates the risk of breaking existing implementations of the interface when new methods are added to it. There is no such need for maintaining backwards compatibility when you're defining new interfaces.

Another smell that I'm detecting here is over-engineering. With very little code, you've managed to whip up three interfaces already and not even much behavior to show for it. Are you writing high-level tests? What do those tests look like? If you don't even have any tests, then that's one reason you're starting to go off into the weeds and heading down the rabbit hole of complexity.

[Emma Sophia Jones]

So my design approach is based around this tutorial: http://www.cokeandcode.com/info/tut2d.html

With a couple of modifications. Basically he was reversing the direction of aliens using a doLogic() method to loop through each of the aliens and change their speed when one of them hit a wall. I didn't like the sound of that and wanted to include two additional classes: AlienRow (made up of, say, 10 aliens) and AlienBlock (made up of 5 rows).

AlienBlock would move, (ie have its X and Y value update) while the rows just have non-changing values for how offset they are from the Block X/Y. Aliens have static values for how offset they are from the X/Y of the parent row.

So from that you have

AlienBlock (movable entity)
AlienRow (entity)
Alien (graphic entity)

Plus the ship and bullets

Ship (movable, graphic entity)
Bullets (movable, graphic entity)

Most of the graphics stuff is handled by a Sprite class, but graphic entities would include a variable for an entity's Sprite and a draw method (which just calls the draw method on the Sprite class).

It's a little hard to post code samples as what I have so far is written around the tutorial's design rather than my own. But I can try if it'd help you guys to understand what I was going for.

Open to criticisms if you have suggestions for how I should improve the design.

[Junilu Lacar]

Your posts have been about high-level design ideas. On the surface, any reasonable idea sounds good. The devil is in the details though and the detailed design is in the code. Without code, it's hard to make calls on your design; all we can do without details is call out smells, which is really all that we've done so far. Smells are just indications that something might be off and needs changing.

[Emma Sophia Jones]

Fair enough, will have a go at writing it the way I have planned then post back so that you guys can tear the approach to shreds ;)

[Emma Sophia Jones]

Okay, all done with a basic version. This is with the class extension approach that I know isn't ideal but was the quickest for me to put together (because I already understand it) for the sake of giving you guys something to review.

Project is a little too big to post in code tags now so I've pushed it to GitHub. If you get chance to review it it'd be really helpful to know your thoughts:

https://github.com/EmmaO/SpaceInvaders101

[Junilu Lacar]

You're obviously not a total noob since your code is relatively well organized. It looks to me like you've got prior programming experience in a different language.

The comments you have in place right now are unconventional.  Read about how to write JavaDocs and make your block comments conform to those conventions.

The Constants.java you wrote is a big no-no. That's a telltale sign of someone transitioning from a more procedural programming background. Put each constant in the class that it pertains to the most. For example, SHIP_SPEED should be with the Ship class, not mixed in with a bunch of unrelated constants. The organization of well-factored OO programs is such that things that are used together are together.  You're going to have to break the way you thought about program organization in the procedural world and adopt an entirely different world view for OO programs.

Speaking of ship, this code:
// in Game.java ... if(leftPressed){ ship.setXSpeed(-Constants.SHIP_SPEED); } else if (rightPressed){ ship.setXSpeed(Constants.SHIP_SPEED); } else { ship.setXSpeed(0); } ship.move(delta);
is very procedural rather than OO and is a direct result of you using the disembodied Constants.SHIP_SPEED.

Consider this alternative:
// in Game.java ... ship.setDirection(shipDirection()); ship.move(delta); ... /** * Returns a Direction indicating which way the ship should be made to move. */ private Direction shipDirection() {   if (leftPressed) return Direction.LEFT;   if (rightPressed) return Direction.RIGHT;   return Direction.NONE; } // Direction.java public enum Direction {   LEFT(-1), RIGHT(1), NONE(0); // maybe also UP(-1), DOWN(1) ?     private int direction;   private Direction(int direction) {       this.direction = direction;   }   /**    * Turns the given speed into a velocity by    * adding the appropriate sign for this direction.    */   public int velocity(int speed) {        return direction * speed;   } } // in Ship.java: private static final int SPEED = ... ;  // moved here from Constants private Direction direction = Direction.NONE;  // default: not moving public void setDirection(Direction dir) {    this.direction = dir; } public int getVelocity() {    return direction.velocity(SPEED); }
One of the first reactions you might have is : "Wow, that's a lot of code! And so many method calls to eventually multiply -1 to SPEED! Seems very inefficient."

I would say: OO is about behaviors and organization. Programmers suck at optimizing performance by intuition and gut feeling. Don't sacrifice OO for performance because 98 out of 100 times, your gut feeling about performance is unfounded.

[Junilu Lacar]

As far as your original question goes, the way you broke down responsibilities seems reasonable. However, the AlienEntity class is now very anemic and I don't know if it's even necessary. The AlienRow class is where most of the logic resides. I would probably consider eliminating AlienEntity and refactoring AlienRow a little bit.  The only interesting behavior AlienEntity provides is to track its offset within an AlienRow and then report whether it has been hit by a Shot. The refactoring would again start with moving a value out Constants.java, namely, Constants.ALIEN_OFFSET_X.  The class that uses that is the AlienRow class so this is where I'd move it first. Then I'd ask, why isn't the AlienEntity more interested in this value instead? Why doesn't it have a reference to the offset? The answer to that question (I'll let you reason it out for yourself) is what leads me to think that AlienEntity is pointless and can be eliminated.

You still have a lot of procedural-minded code. This for loop in AlienRow:
for(String key : aliens.keySet()){ if(aliens.get(key).checkForCollision(shot)){ return true; } }
could just be:
for (AlienEntity alien : aliens.values()){ if (alien.checkForCollision(shot)){ return true;       } }
When you eliminate AlienEntity, this really could be just:
for (int nextSpotWithAlien : occupiedPositions){ if (shot.hits(nextSpotWithAlien, getVerticalPosition()) { markAsHit(nextSpotWithAlien); return true;       } }
The logic: AlienRow knows the verticalPosition of all aliens in this row. It should also keep track of all positions within the row that still has an Alien in it. It should tell the Shot which horizontal and vertical positions are potential targets. When the shot reports that it will indeed collide with something at a particular location (by checking its own position against the given position), then you can trigger the behavior/event that "kills" the alien at that position.

[Junilu Lacar]

You should consider changing your formatting style a little bit. I notice that you like to write this kind of code:

// these look like method calls
for(...) {
   ...
}

if(...) {
   ...
}

Those look more like method calls rather than the flow control statements that they are. It's more common practice to put a space after keywords like for and if.

// more conventional way
for (...) {
   ...
}

if (...) {
   ...
}

[Junilu Lacar]

Also, carefully consider names and how your choices affect the code's readability. Which of these makes more sense semantically?

if (alien.checkForCollision(shot))

// or 

if (alien.isHitBy(shot))

The difference between these two lines:
- Syntactically: nothing
- Semantically: everything

Both versions reveal something about the mindset of the programmer when they wrote that code. The first version reveals a very algorithm-oriented focus. The second reveals a focus on intent. In other words, the first is about implementation while the second is about abstraction.

[Junilu Lacar]

Extending that thought a little bit, abstracting the "hit" logic even further and refactoring more could lead you to be able to write this:

for (AlienRow row : allRows) {
    if (row.isDecimatedBy(shot)) {
         eliminate(row);
    }
}

As opposed to whatever equivalent code you have in AlienBlock right now. Doesn't the code above tell a more abstract and clear story?

That would mean that instead of having a checkForCollision(shot) in AlienRow, you'd have something like this instead:

public boolean isDecimatedBy(ShotEntity shot) {
    for (int nextPosition: allOccupiedPositions) {
        if (shot.hits(nextPosition, getVerticalPos()) {
             kill(nextPosition);
             return allOccupiedPositions.isEmpty();
        }      
    }
    return false;
}

Notice that I also refactored the markAsHit() name that I suggested previously to a more intent-focused name, kill(). That's the thing with us programmers, we always seem to be focused on implementation even when we try not to be. It takes a lot of discipline and awareness to go back and find everything that is affected by the implementation-focused mind and change it to something that is more intent-focused.

[Emma Sophia Jones]

Hi Junilu,
First up, thanks for taking the time to go through everything I posted (and to then type up that response). Really kind of you.

In terms of prior experience I have done some stuff with VBA before but most of my programming education does come from Java teachers believe it or not. I did the Stanford online course for Java and then C++ which was sort of based around teaching generic, good programming methodology. I finished them off last year but the courses actually ran back in 2008 I think… so it’s possible that standard practices have changed since then… or maybe those teachers themselves have just struggled to shake off procedural habits (which they’ve since passed to me). The other source of knowledge for this project was the tutorial I posted a bit earlier on – again I guess maybe that guy had a procedural background.

Having read through your posts there’s some stuff that I agree with and can take on board pretty easily:
JavaDocs – yeah I am not approaching these correctly, will read through the link you posted and try to take it on board
If/For statement formatting – happy to change style on this if that’s the standard approach
Method names – I will try to adopt more of an implementation mindset with these… as you say it’s tricky but I see the advantages
Looping through a HashMap – Pretty much didn’t know that the Syntax you suggested was an option. I agree it’s more readable though, will use that going forwards.

There’s  some other stuff though where I don’t fully understand the rationale behind the approach you’re suggesting. Hopefully it doesn’t come across as combative but it’d be helpful to understand why you feel what you’ve suggested works better:

Constants
The Constants Class is something I (basically) picked up from the Stanford course. It was an element included in virtually all of the assignments they set. I have a little more trouble letting go of this as, especially with a game, I can see value to having all elements which we may want to adjust to tweak gameplay in a single place.

If I want to make the game a bit easier it’s just nice to be able to jump to the Constants class then reduce the number of aliens, make the aliens move slower and speed up the bullets a bit.

Could you explain the advantage to not using a Constants class?

Alternative ship movement
So my method for ship movement isn’t actually my code and I’m not especially defensive about it in that sense. But. I can see the ship movement is a little procedural currently but it’s also clear and concise. My concern with the lengthier one which you suggested isn’t so much that it may be less efficient – more that it seems less readable to me. If I showed the former to someone who couldn’t code I think they’d be able to understand (especially with a few comments thrown in), the latter… I think they’d struggle to follow. Can you explain what the advantage is to the code which you suggested?

Removal of AlienEntity
It’s interesting that you suggested the removal of AlienEntity as my instinct would have been to remove AlienRow if we were cutting down on classes. AlienRow pretty much serves only to pass messages between AlienBlock and AlienEntity…. And track the Y position of the child AlienEntities.

AlienEntities though strike me as kind of important as they provide a graphic representation of the aliens on screen. (They also seem to be the more natural object to ask hasBeenHitBy(ShotEntity shot))
Despite thinking that AlienRow wasn’t strictly necessary, I decided the neatest way to divide the roles was to have AlienEntities be the things you see on screen which get hit. And then organise them into rows and organise the rows into blocks. It added more code ultimately but my thought was that this would make it more readable.

So I guess my questions are. Is there an inherent advantage to removing classes? If so, I suppose I don’t really understand why you’d choose to get rid of AlienEntity over AlienRow. You mentioned that AlienEntity doesn’t make reference to the Constants.ALIEN_OFFSET_X… which is true. But it does store a value to say how far it is offset from the row X and that value is calculated when the AlienRow constructs it by
positionInRow * ALIEN_OFFSET_X



Hopefully those questions are clear. Thanks again for your time working on this – I’ve not actually had my code reviewed by another programmer before and the feedback is really useful.

[Junilu Lacar]

I'll address your questions, one per reply.

Regarding a Constants class. This is a very procedural construct. As I said, object-orientation is mainly about behaviors and responsibilities. A class like Constants has no behavior. It's just a namespace for global constants and therefore a poor excuse for a class. Do you see a Constants class in the standard library? There is none. Any public (global) constant values that are defined by the standard library are defined in the classes that they are most closely associated with. Like Integer.MIN_VALUE and Integer.MAX_VALUE. If you think that these make sense, then why wouldn't you define Ship.SPEED instead of Constants.SHIP_SPEED?

A Constants class emits a code smell: the author didn't or couldn't decide to which class(es) the various values that are defined within are most closely associated. A Constants class is a cop-out at best and lazy at worst. The fact that you even have comment headers for each section of values in the Constants class screams "REFACTOR ME!"  ALIENS_PER_ROW wants to be part of the AlienRow class, for example. NUM_ROWS wants to be part of the AlienBlock class.

Again, if you're going to write an OO program, you're going to have to recognize what constructs are more procedural than OO. Why would you have code external to a Ship know about a ship's speed? What you're doing when you write something like ship.setXSpeed(-Constants.SHIP_SPEED); is externalizing the logic/responsibility of determining the ship's movement.  The *ship* should know how to manage its movement in an OO program. But the Constants.SHIP_SPEED makes it difficult to break away from the mindset of telling a Ship what its speed is because if Constants.SHIP_SPEED is a "good practice" then certainly ship.setXSpeed(-Constants.SHIP_SPEED); can't be that bad either, right? Well, it is if you want to write OO code.

It's like me telling you how fast you should type or how much milk you should put in your bowl of cereal. It's really none of my business to tell you those things. Likewise, it's really none of your (the external code) business to tell a Ship how fast it should move.  If a Ship has a constant speed, then that constant should be defined in Ship.  If you get used to that kind of thinking, then it becomes more intuitive when someone asks, "How fast does a Ship move?" to answer, "I don't really know but if we look at Ship, it will probably lead us to the answer." That's what you'd expect an Object-Oriented programmer to think. It's more of a procedural programmer who would say, "I don't know but there must be a global constant for that defined in a Constants class somewhere."

[Junilu Lacar]

Let's examine these lines of code:

if (leftPressed) {
   ship.setXSpeed(-Constants.SHIP_SPEED);
} ...

What does the negative mean? Is that really readable? It's not. How can speed be negative? Can anything move slower than not moving at all? You might say that "Well, you're just being stupid. Any reasonable person should know that -Constants.SHIP_SPEED means that the ship will move to the left while a positive Constants.SHIP_SPEED means that the ship will move to the right." Any reasonable person who knows that you are going to do math with those values might be able to infer that, sure, but it's better to be more explicit.

You said that the code I proposed is less readable. Maybe for someone who's not used to thinking with an OO mindset. The code I propose is actually more abstract and it's more explicit. There is no implied knowledge like "-1 means moving left and +1 means moving right, 0 implies not moving." The Direction enum is defined as either LEFT, RIGHT, or NONE.  When you write

ship.setDirection(shipDirection());

you don't have to care what the details of the calculations are, you just need to know that direction can be either left, right, or none. In fact, you can change the details of how a ship's direction is determined at any time without affecting this code. The code doesn't need an if-statement either because as far as this code is concerned, there is no condition involved in determining the ship's direction. That's the shipDirection()'s business to know. The code at this level of abstraction doesn't need to bother itself with such details. What if the input device was changed from the keyboard to a mouse or a joystick?  In that case, you'd have to change the conditional in your code to something like

if (mouseMovingLeft) ...

or 

if (joystickLeft) ...

This is not good because you haven't abstracted away those details but instead tied them in tightly with the logic to determine the speed (and implied direction) of the ship.

[Junilu Lacar]

If I showed the former to someone who couldn’t code I think they’d be able to understand (especially with a few comments thrown in)...

Comments are often a code smell. They are an indication that the code could be made more clear or expressive.

So here's how I'd imagine the conversation to be for the code you say is more understandable:

You: "Read that code and tell me what you think it does"

Other: "If left pressed then you set the ship's XSpeed to negative Constants.SHIP_SPEED. If right pressed you set the ship's XSpeed to Constants.SHIP_SPEED, otherwise you set its XSpeed to 0. Ok, so negative speed means left and a positive speed means right and 0 means it's not moving at all. Ok, I understand."

You: "Ok, read this other version and see if it makes more or less sense to you."

Other: "This code just sets the ship's direction to, well, what's this shipDirection() thing?"

You: "Oh, that's a method. It returns a Direction value."

Other: "And what's a Direction value?"

You: "LEFT, RIGHT, or NONE."

Other: "Ok, so you're setting the ship's direction to whatever direction the shipDirection() method returns. Cool."

You: "So that makes sense?"

Other: "What, to set the ship's direction to LEFT, RIGHT, or NONE? Why wouldn't it make sense?"

EDIT: Added a little confusion to Other's lines.

Of course, this is a conversation that I cooked up in my biased mind. What conversation do you have in your mind?

[Junilu Lacar]

Emma Sophia Jones wrote:
Removal of AlienEntity
It’s interesting that you suggested the removal of AlienEntity as my instinct would have been to remove AlienRow if we were cutting down on classes. ... So I guess my questions are. Is there an inherent advantage to removing classes?

The goal is not to cut down on classes. In fact, when you're refactoring to clean up a design, there's a good possibility that you'll end up with more classes/methods than you started out with. This is because we humans seem to have a natural tendency to lump a lot of things together that shouldn't be lumped together in a well-structured OO program.  If you read Martin Fowler's book on Refactoring, the first chapter's long refactoring example (I think it goes for more than 20 pages if I remember correctly) is exactly like that.

AlienEntities though strike me as kind of important as they provide a graphic representation of the aliens on screen. (They also seem to be the more natural object to ask hasBeenHitBy(ShotEntity shot))

That could very well be the case. You might be missing the point though. The point of my replies was to give you an alternative way to look at your design. I'm not saying that the alternative I propose is the best either but that's not important. The thought process and mindset you use to reason and decide is what's important. The main question to ask is this: Whose responsibility is this? Who is responsible for knowing and using a Ship's speed? Who uses and is responsible for applying the ALIEN_OFFSET_X value? Is access to this value as limited as it could possible be? Obviously, when ALIEN_OFFSET_X is in a global constants class, access to it is wide open.

Why would you want to limit access and scope? Because the more access you give to something the more things will want to access it, naturally. The more things that access something, the more things you have to check if something related to that widely-accessed thing changes. The more things you have to check, the more of a pain in the a$$ it is to maintain that part of the code. Also, the more things are dependent on each other, the more likely it becomes that any small change will break some existing functionality.  

Minimizing access and narrowing the scope of dependencies gives you more control over your program. One habit I've developed is to try to give methods as limited access as I can first. The fewer public methods I have, the fewere dependencies I have to worry about and test.  Fewer dependencies means there are fewer things that can break and fewer things that a bug can ripple out to and affect. These are the advantages of high cohesion in code and loosely coupled code

[Junilu Lacar]

Emma Sophia Jones wrote:
Method names – I will try to adopt more of an implementation mindset with these… as you say it’s tricky but I see the advantages

I was hoping you'd adopt more of an intention-focused mindset after reading everything I wrote...

[Junilu Lacar]

Emma Sophia Jones wrote:I suppose I don’t really understand why you’d choose to get rid of AlienEntity over AlienRow.

As before, the intent of my reply was to get you to think about an object as a collection of related behaviors and capabilities/responsibilities. If code were to be moved out of AlienEntity because it had more affinity with data that was managed by AlienRow, then there wouldn't be much left in AlienEntity. A class with little or no behavior is called an "anemic class" and its existence in an OO design is suspect. If you want to keep the behavior/responsibility of drawing an image on a graphics canvas in the AlienEntity, that's fine. I suppose that would qualify as interesting behavior. Getter and setter methods don't qualify, unless they do some complicated calculations and not just return or mutate private fields.

[Piet Souris]

Junilu Lacar wrote:I'll address your questions, one per reply.

Interesting!

Junilu Lacar wrote:
(...)A Constants class emits a code smell: the author didn't or couldn't decide to which class(es) the various values that are defined within are most closely associated. A Constants class is a cop-out at best and lazy at worst. The fact that you even have comment headers for each section of values in the Constants class screams "REFACTOR ME!"  ALIENS_PER_ROW wants to be part of the AlienRow class, for example. NUM_ROWS wants to be part of the AlienBlock class.

Again, if you're going to write an OO program, you're going to have to recognize what constructs are more procedural than OO. Why would you have code external to a Ship know about a ship's speed? What you're doing when you write something like ship.setXSpeed(-Constants.SHIP_SPEED); is externalizing the logic/responsibility of determining the ship's movement.  The *ship* should know how to manage its movement in an OO program. But the Constants.SHIP_SPEED makes it difficult to break away from the mindset of telling a Ship what its speed is because if Constants.SHIP_SPEED is a "good practice" then certainly ship.setXSpeed(-Constants.SHIP_SPEED); can't be that bad either, right? Well, it is if you want to write OO code.

Last year I experimented too, like Emma, by having a dedicate class full of constants. I had never done that before, and I was wondering whether this approach is easy to use while developing some project. Never had I the idea of anything 'code smell',  but admitted, your nose for this is second to none and much more developed than mine.

The difference with Emma is that I used enums. As an example, this is the beginning of my constants class:

public class SudokuConstants {

    enum GameType {NORMAL, KILLER, CALCU}
    enum GroupOperation {NORMAL, SUM, PROD, DIV, MINUS}
    enum GroupType {ROW, COL, SQUARE, OTHER}
    enum GameStatus {SETUP, USERSOLVING, FINISHED}
    
    enum Colors {
        NORMAL (new Color(175,236,111)),
        SELECTED(new Color(255,255,231)),
        GROUPED(new Color(225, 255, 160)),
        BUTTONPANEL(Color.BLUE),
        KILLERCELLPANEL(Color.BLACK),
        MESSAGEPANEL(Color.LIGHT_GRAY),
        GROUPBORDER(new Color(0, 200, 0)),
        CURRENTCELL(Color.CYAN)
        ;
        
        final Color color;
        
        Colors(Color c) {
            color = c;
        }
    }
    
    enum Message {
        SETUP ("Press \"Set Up\" to start the set up"),
        USERSOLVING ("Press \"User Solving\" when done set up"),
        WRONGNUMBEROFCELLS ("nr of cells > size!"),
        .....

et cetera.

Now, I have mixed feelings about all this. It is handy while developing, having all these enums together makes for an easy overview and maintainability. But using it in the actual code is pretty clumsy, a lot to type.
And no, I do not do this because I have no idea in what class to store the information!  

But the points you make are certainy valid, and when I start another project, I doubt I will use this approach again.

Just one final remark: if you have a class 'Ship', with a constructor that sets the speed, then you have still the situation that something outside tells the ship its speed.

[Junilu Lacar]

Thanks for your comments, Piet.

I should caution against taking what I said out of this particular context and trying to apply it elsewhere: It may or may not be appropriate to do so. With regards to the smell of something outside of Ship telling it what its speed should be, there may be contexts in which something like that might be appropriate. In this case, there are other ways to orchestrate the interaction that doesn't require "micromanaging" knowledge of implementation details like SHIP_SPEED.

Let's take the example of an unmanned spacecraft. On launch, the vehicle needs to attain escape velocity. I'm not saying this is the way the actual control systems are designed but I could imagine that it would be easier for ground control to send the desired escape velocity and then let the vehicle's onboard computers handle the details of adjusting everything else that needs to be adjusted in its propulsion system to achieve the desired velocity. So again, I think it really goes back to simplifying (the interface/interaction) and hiding details, i.e., abstraction.

I guess the smell I detected in the OP's code was "micromanaging knowledge externally" which indicates a lack of abstraction and simplicity of the object's API.

[Piet Souris]

Junilu Lacar wrote:Thanks for your comments, Piet.(...)

You're welcome!

And have a cow for all your quality explanations!

[Emma Sophia Jones]

Okay cool. So the constants class I think I get now. I think that in this particular case, taken in isolation, the constants class actually is the more convenient way to manage the different game attributes that I want to keep track of. That said I can see times when it wouldn’t be the case (if the project was much larger for example) and I can see the benefits of maintaining a consistently object-oriented approach (helps other people who may want to fiddle with the code amongst other things).

Your dialogue script made me laugh a bit. To be honest I showed both pieces of code to my partner and she didn’t really get either but seemed more comfortable looking at the shorter piece of code. At the end of the day that short one requires you to work out two lines of code then reapply the knowledge a few times… the longer one you need to understand a lot more so the barrier for entry is higher. But, setting aside readability for non-programmers, I agree that the ship should be responsible for setting its own speed and that it makes more sense for Game to issue either direct commands (Direction = LEFT) or keystrokes (leftPressed) to the ship… then leave it to the ship to handle that information.

I think I’ll probably keep the three different Alien classes – I’m pretty comfortable with the separation of responsibilities there and think that AlienEntity probably doesn’t qualify as anemic based on your description. I think I’m gonna take another pass at the project though, remove the constants class and refactor a bit.

Thanks again for the help Junilu.

[Junilu Lacar]

If you're going to get into refactoring, one important skill to have is detecting code smells.  Martin Fowler's book, Refactoring: Changing the Design of Existing Code has, in my mind, three parts: Chapter 1 is a long refactoring example. The next part is a catalog of code smells (20+ of them). The remainder of the book is a catalog of refactorings that you can apply to improve the design of your code.

When I first read the long refactoring example in the first chapter of that book, I was blown away. I had about seven years of professional experience by then and thought that I wrote pretty clean code. The refactoring example made me realize just how far I still had to go but it was also encouraging because I was already practicing many of the techniques in the catalog. The only difference was that I did not yet have the habit of writing automated unit tests nor did I have the kind of vision and sense of smell that was necessary to see the big picture.  I felt like someone who only knew how chess pieces moved and maybe was able to muddle through a game, watching a Grandmaster actually play the game with strategy and finesse. Refactoring requires a higher level of understanding of what makes a design good or bad and the discipline to always write tests, identify smells, and systematically remediate design flaws.

I mentioned unit tests a couple of times. This is important. You shouldn't try to refactor too much without a good set of unit tests to back you up. Unit tests are your safety net. I'm a firm believer in Test-Driven Development and I believe it is the catalyst that blends clean code, good design, refactoring, and unit testing into a holistic and synergistic practice.