耐心游戏(克朗代克)

Question

我一直在使用javaFX在java中开发一个耐心克隆。

它还没有完成，有些特性不起作用，还有一些bug仍然存在，但我真的需要一些结构性建议，我知道我正在做的很多事情都变得非常混乱。

如果有人有足够的知识和时间给我提供任何建议，说明我可以做什么不同的事情，以及如何改进我的代码，我会非常感激的。

欢迎任何反馈意见:)

节目的图片：

这是指向我的github：https://github.com/vincent-nagy/Card-Games的链接

这是游戏的控制器，因为我需要发布一些代码与这个问题。所有其他类都可以在github上找到，因为github太多了，无法在这里发布。

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);
        }
    }
}
}

Answer 1

保持简单

公共空addEventHandlers(节点节点，布尔doNow){

这是个奇怪的界面。同样的方法

1. 将节点放入队列以供以后处理，
2. 处理排队的节点，或
3. 立即处理节点。

为什么？

考虑制定三种方法。每个目的一个。然后，您可以调用正确的参数，而不是传递不同的参数。正如最初所写的，如果您意外地用空node调用该方法，它将立即清除队列。

经验法则是一种方法应该做一件事。这种方法有三种不同的行为。两个太多了。

addEventHandlers(null, false);

这能做什么？一方面，你告诉它不要处理节点，另一方面，你告诉它要处理所有节点。正如所写的，处理所有节点获胜。但是要想弄清楚这一点，我们必须阅读该方法的代码。

幻数

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

为什么是2？如果你重组会发生什么？这段代码如何知道您在其他地方做了更改？猜测一下，您在这里真正想做的是getTopCardPane()。当然，这取决于如何安排其余的代码。

您也有4和7，虽然我不太清楚这些在此代码之外是否重要。

说你的意思

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

注释与代码的功能不匹配。该评论讨论了当它为空时该做什么。当它不是null时，代码会处理这个情况。为什么不照评论说的做呢？

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

现在你不需要评论了。代码是不言自明的。

当然，更大的问题是，您正在通知用户编程错误。用户应该如何处理这些信息？在交易过程中，你犯了一个错误，出示了一张空牌。这是士兵的密码。

一个更好的解决方案是，如果这种极端奇怪的情况真的发生，就结束这个程序。然后，用户将报告一个空指针异常。如前所述，用户可能会报告一个空牌错误，或者只是他们不能玩游戏。因为代码在遇到致命错误后试图继续运行。

考虑一下，如果你不做防御性检查的话，它会简单得多。启动的代码块

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

可能是

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

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

现在，我们不再检查每一次迭代，而是在迭代之后移动该行为。我们不需要有条件，因为我们总是这样做一次。

如果你还想要防御性的检查，那你就应该退出。这是无法恢复的。

基于实现的

接口

私有ArrayDeque eventHandlerQueu =新的ArrayDeque<>()；

可能是

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

然后，我们不必担心它在任何地方都是什么样的Deque，但在这里除外。如果我们想在未来改变，没有人需要知道。

不要重新发明车轮

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

当队列不是空的时候，您想要迭代。所以说

            while (!eventHandlerQueu.isEmpty()) {

不要获取然后丢弃的值。特别是因为它执行相同的isEmpty检查来返回null。因此，这不仅可读性较低，而且可能效率较低。