Return to Revisions

• OOP question

I have created new classes for a loss screen and win screen. They of course both only need to be instanced once, and they always look the same, should I not have used OOP for this?

When coding in Java you should always use OOP.

But OOP does not mean to split up code into random classes. Doing OOP means that you follow certain principles which are (among others):

• information hiding / encapsulation
• single responsibility / separation of concerns
• same level of abstraction
• prefer polymorphism over branching
• KISS (Keep it simple (and) stupid)
• DRY (Don't repeat yourself)
• "Tell! Don't ask."
• Law of demeter ("Don't talk to strangers!")

In Java the classes concept supports this principles but OOP is not bound to classes (except polymorphism).

You create classes when there is different behavior, where behavior means communication with dependencies (aka calling methods on other objects) or calculations.

Your classes for a loss screen and win screen might have different behavior (ie. adding different components to the screen). Then having separate classes would be OK.

If they just display different strings in different colors/sizes there should be only one class with a parametrized constructor.

• Game logic

IMHO there a tree major problems with your code:

1. poor naming
2. code duplication
3. methods with status return and side effects.

Naming

Finding good names is the hardest part in programming. So always take your time to find good names.

Although you follow the general Java Naming Conventions your identifier names could be improved in several ways. The fact that you have to explain your methods in prose in your question indicates that.

• Choose names from the problem solution domain, not from the programming domain.

• Let method names start with a verb.
  Methods are actions on an object. Having a verb as the first name part improves the readability of your code greatly:
  gameLogic.genericMove() vs gameLogic.move()

• boolean variables and methods returning a boolean should be prefixed with is or has

• Do not fear to be verbose.
  There is no penalty for long identifier names. (actual IDEs even prevent you from typing them more than once via code completion...) So resist to artificially shorten them.

code duplication

You have similar code to move your tiles into different directions.

By applying OOP you could move the differing calculations in each of that methods into classes implementing the same custom interface. Then you'd have only one method doing the loops getting the interface type as parameter. The differences for each direction would be encapsulated in classes of their own implementing this interface (which could be called Direction).

Please see this answer for more details: Simplified version of 2048 game

This way you would safe 3/4 of the "similar code".

Methods with status return and side effects

Methods should either return a (calculated) value or change the state of an object. Your "logic" methods do both.

Looks like the main reason for this is that you do your logic twice: first for checking the possibility and second for actually doing the move.

You could choose a different approach where you do the move in a copy of the board and simply exchange the "real" board with the copy if the move succeeded. In this scenario the game would have a member variable hasMoveSucceeded:

private boolean move(Direction direction) {
 pause();
 updateIndices();
 moveInCopy(direction);
 if (hasMoveSucceded) {
 exchangeBoardWithCopy();
 addNewTile();
 checkState();
 return true;
 } else {
 // resume(); // maybe obsolete?
 return false;
 }
}

I do not how I would implement the different for loops in a parameterized method, some of the for loops are ascending and others are descending. – The Coding Wombat

the interface:

interface Direction{
 int startValue();
 boolean hasNext(int loopIndex);
 int next(int loopIndex);
 int getX(int loopIndex);
 int getY(int loopIndex);
}

the directions as anonymous inner classes:

Direction right = new Direction(){
 @Override public int startValue(){
 return tiles.length - 2;
 }
 @Override public int hasNext(int loopIndex){
 return loopIndex >= 0; 
 }
 @Override public int next(int loopIndex){
 return loopIndex--;
 }
 @Override public int getX(int loopIndex){
 return loopIndex;
 }
 @Override public int getY(int loopIndex){
 return 1;
 }
}

next two values just guessed, hope you see the idea...

Direction left = new Direction(){
 @Override public int startValue(){
 return 0;
 }
 @Override public int hasNext(int loopIndex){
 return loopIndex < tiles.length - 2; 
 }
 @Override public int next(int loopIndex){
 return loopIndex++;
 }
 @Override public int getX(int loopIndex){
 return loopIndex;
 }
 @Override public int getY(int loopIndex){
 return 1;
 }
}

Direction down = new Direction(){
 @Override public int hasNext(int loopIndex){
 return loopIndex < tiles.length - 2; 
 }
 @Override public int startValue(){
 return 0;
 }
 @Override public int next(int loopIndex){
 return loopIndex++;
 }
 @Override public int getX(int loopIndex){
 return 1; 
 }
 @Override public int getY(int loopIndex){
 return loopIndex;
 }
}

the loop method

private void move(Direction direction) {
 tileMoved = false;
 for (int i = direction.startValue(); direction.hasNext(i); i=direction.next(i)) {
 if (tiles[i] != null && tiles[i].isExists()) {
 if ((i + 1) % BOARD_SIZE != 0) {
 tileMoved = genericMove(direction.getX(i), direction.getY(i), MOVE_DISTANCE, 0);
 if (tileMoved) {
 move(direction);
 break;
 }
 tileMoved = genericUpgradeMove(direction.getX(i), direction.getY(i), MOVE_DISTANCE, 0);
 if (tileMoved) {
 move(direction);
 break;
 }
 }
 }
 }
}