Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BuildingCardFactory #148

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

chiashengchen
Copy link
Collaborator


public interface BuildingCardFactory {

public enum Type {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

卡牌名稱應該不能當作 type,建築物的名稱不算是常數的概念,只是建築卡有一個 name 的屬性

}
}

public enum Color {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BuildingCard 才會有 color ,enum 不會放在 factory 這邊

@@ -83,22 +82,46 @@ public static class UserRequest {
private List<BuildingCard> getBuildingCards() {
List<BuildingCard> cards = new ArrayList<>();
Stream.of(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我覺得應該把創建卡牌的邏輯開一個類別來負責處理,usecase 只要負責去使用就好

@snowmancc
Copy link
Collaborator

我有把類別圖放在 issue 那邊,可以討論一下

@Data
@AllArgsConstructor
public class BuildingCard {
String name;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

此BuildingCard 類別可能要封裝屬性只能Get ,因為建築卡應該是不會被改變的~

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

可以使用 @value 對嗎

Copy link
Collaborator

@timt1028 timt1028 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants