3
\$\begingroup\$

I've been working on a Patience clone in java using javaFX.

It's not finished yet, there are some features not working and some bugs still there, but I'm really in need of some structural advice and I know that alot of the things I'm doing are just getting real messy.

If someone has the knowledge and time to give me any advice on what I could do differently and how it would improve my code, I'd really appreciate it.

Any feedback is welcome :)

An image of the program: The patience game running

Here is the link to my github repo: https://github.com/vincent-nagy/Card-Games

Here is the controller of the game since I need to post some code with this question. All other classes can be found on the github since it's too many to post here.

package be.vincent_nagy.cardgames.java.controller;

import be.vincent_nagy.cardgames.java.controls.CardPane;
import be.vincent_nagy.cardgames.java.event.DragDetectedHandler;
import be.vincent_nagy.cardgames.java.event.DragDoneHandler;
import be.vincent_nagy.cardgames.java.event.DragDroppedHandler;
import be.vincent_nagy.cardgames.java.event.DragOverHandler;
import be.vincent_nagy.cardgames.java.model.*;
import javafx.event.ActionEvent;
import javafx.fxml.FXML;
import javafx.scene.Group;
import javafx.scene.Node;
import javafx.scene.control.Button;
import javafx.scene.image.ImageView;
import javafx.scene.input.MouseEvent;
import javafx.scene.layout.AnchorPane;
import javafx.scene.layout.StackPane;

import java.util.ArrayDeque;


public class PatienceController {
@FXML
private AnchorPane tablePane;
@FXML
private StackPane nextCardStackPane;
@FXML
private Button start;
@FXML
private Group stackGroup;
@FXML
private ImageView nextCardButton;

private Deck deck;
private Table gameTable;
private Stacks stacks;
private NextCards nextCards;
private ArrayDeque<Node> eventHandlerQueu = new ArrayDeque<>();

public void initialize(){
    start.setOnAction(this::startGame);
    nextCardButton.setOnMouseClicked(this::showNextCards);
}


private void startGame(ActionEvent event) {
    //Empty out previous things
    tablePane.getChildren().removeAll(tablePane.getChildren().filtered(e -> e instanceof CardPane));
    nextCardStackPane.getChildren().clear();
    stackGroup.getChildren().clear();

    //Create a new gamefield
    createGameField();

    addEventHandlers(null, false);
}

/*
    Create everything needed to play the game
 */
private void createGameField() {
    //create stacks
    createCardStacks();
    //deal the cards on the table
    dealCards();
    //Create and show next cards
    initNextCards();
}

/*
    Create the stacks on which the cards should be stacked to finish the game.
 */
private void createCardStacks() {
    //Create 4 stacks on which cards are to be stacked from 1 to 13 to finish the game
    stacks = new Stacks();

    //Add events and add to the stackgroup
    for (int i = 0; i < 4; i++) {
        addEventHandlers(stacks.getStack(i), false);
        stackGroup.getChildren().add(stacks.getStack(i));
    }
}

/*
    Deal the cards on the table and set them up
 */
private void dealCards() {
    //Create a new deck which gets shuffled automatically
    deck = new Deck();

    //Create a new table which creates 7 columns which contain 20 empty CardPanes each
    gameTable = new Table(this);

    //Add the CardPanes from whole table to the field.
    for (Column c: gameTable.getColumns()) {
        for (CardPane cp : c.getCardPanes()) {
            tablePane.getChildren().add(cp);
        }
    }

    //Pull the cards from the deck and show them on the table. Each column has 1 more card than the previous
    for(int i = 0; i < 7; i++){
        for (int j = 0; j <= i; j++) {
            Card drawnCard = deck.playCard();

            //Shouldn't be null but if it is, show a message
            if(drawnCard != null){
                gameTable.setCard(i,j,drawnCard);

                //Make the card shown if it's the last card in the column and add events
                if(j == i){
                    gameTable.setShown(i, j, true);
                    addEventHandlers(gameTable.getCardPane(i,j), false);
                }
            } else{
                System.out.println("Deck is empty error");
            }
        }
    }


}


//Load and show the next cards in the created card stacks
private void initNextCards() {
    nextCards = new NextCards(nextCardStackPane, this);
    nextCardButton.setVisible(true);
    showNextCards(null);
}

private void showNextCards(MouseEvent mouseEvent) {
    nextCards.show(deck);
    addEventHandlers(nextCards.getCardPane(2), false);
}

//Give a node to add drag and drop handlers. Once everything is finished,
// this method gets called with null to ensure the handlers don't called to early
public void addEventHandlers(Node node, boolean doNow){
    DragDetectedHandler dragDetectedHandler = new DragDetectedHandler(stacks, nextCards, gameTable);
    DragOverHandler dragOverHandler = new DragOverHandler();
    DragDroppedHandler dragDroppedHandler = new DragDroppedHandler(stacks, gameTable, nextCards);
    DragDoneHandler dragDoneHandler = new DragDoneHandler(gameTable, nextCards);

    if(!doNow && node != null){
        eventHandlerQueu.add(node);
    } else {
        if(node == null) {
            while (eventHandlerQueu.peek() != null) {
                Node currentNode = eventHandlerQueu.pop();

                currentNode.setOnDragDetected(dragDetectedHandler);
                currentNode.setOnDragOver(dragOverHandler);
                currentNode.setOnDragDropped(dragDroppedHandler);
                currentNode.setOnDragDone(dragDoneHandler);
            }
        } else {
            node.setOnDragDetected(dragDetectedHandler);
            node.setOnDragOver(dragOverHandler);
            node.setOnDragDropped(dragDroppedHandler);
            node.setOnDragDone(dragDoneHandler);
        }
    }
}
}
\$\endgroup\$
2
\$\begingroup\$

Keep it simple

public void addEventHandlers(Node node, boolean doNow){

This is a weird interface. The same method can

  1. Enqueue a node for later processing,
  2. Process the queued nodes, or
  3. Process a node immediately.

Why?

Consider making three methods. One for each purpose. Then instead of passing different parameters, you can just call the right one. As originally written, if you accidentally call the method with a null node, it will immediately clear the queue.

The general rule of thumb is that a method should do one thing. This method has three different behaviors. That's two too many.

    addEventHandlers(null, false);

What's this supposed to do? On the one hand, you're telling it not to process nodes, and on the other, you're telling it to process all the nodes. As written, process all the nodes wins. But to figure that out, one has to read the method's code.

Magic numbers

    addEventHandlers(nextCards.getCardPane(2), false);

Why 2? What happens if you reorganize? How will this code know that you made a change elsewhere? At a guess, what you actually want to do here is to getTopCardPane(). But of course that depends on how you have the rest of the code arranged.

You also have 4 and 7, although it's less clear to me if those matter outside this code.

Say what you mean

            //Shouldn't be null but if it is, show a message
            if(drawnCard != null){

The comment doesn't match what the code does. The comment talks about what to do when it's null. The code handles the case when it's not null. Why not do what the comment says?

            if (drawnCard == null) {
                System.out.println("Deck is empty error");
                continue;
            }

Now you don't need the comment. The code is self-explanatory.

Of course, a bigger issue is that you are notifying a user of a programming error. What is a user supposed to do with this information? You made a mistake and produced a null card during the deal. This code soldiers on.

A better solution is to end the program if this extremely weird situation actually occurs. Then the user would report a null pointer exception. As is, the user might report an empty deck error or just that they stopped being able to play the game. Because the code tries to keep running after encountering what should be a fatal error.

Consider how much simpler it would be if you don't do this defensive checking. The block of code starting

        for (int j = 0; j <= i; j++) {

could be

        for (int j = 0; j <= i; j++) {
            gameTable.setCard(i, j, deck.playCard());
        }

        gameTable.setShown(i, i, true);
        addEventHandlers(gameTable.getCardPane(i, i), false);

Now instead of checking on every iteration if we're done, we move that behavior after the iterations. We don't need a conditional, as we always do it once.

If you still want the defensive check, then you should exit on it. It's not recoverable.

Interfaces over implementations

private ArrayDeque<Node> eventHandlerQueu = new ArrayDeque<>();

Could be

private Deque<Node> eventHandlerQueu = new ArrayDeque<>();

Then we don't have to worry about what kind of Deque it is anywhere but here. If we want to change in the future, no one else needs to know.

Don't reinvent the wheel

            while (eventHandlerQueu.peek() != null) {

You want to iterate while the queue is not empty. So say that

            while (!eventHandlerQueu.isEmpty()) {

Don't fetch a value that you then discard. Particularly since chances are that it does the the same isEmpty check to return null. So not only is this less readable, it's probably less efficient.

\$\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.