Skip to content
This repository has been archived by the owner on Aug 6, 2024. It is now read-only.

4회차 과제 PR #32

Open
wants to merge 7 commits into
base: chjih
Choose a base branch
from
Open

4회차 과제 PR #32

wants to merge 7 commits into from

Conversation

chjih
Copy link

@chjih chjih commented Jul 27, 2022

No description provided.

@chjih chjih changed the title 4주차 과제 4회차 과제 PR Jul 27, 2022
Copy link
Collaborator

@findstar findstar left a comment

Choose a reason for hiding this comment

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

전체적으로 코드가 간결해지고, 알기 쉬워졌습니다.
몇가지 코멘트 남겼으니 추가적으로 고민해보시고 개선해보시면 좋겠습니다.

Comment on lines 18 to 23
private String checkLength(String name){
if(name.length()>5){
throw new IllegalArgumentException("자동차 이름은 5자를 넘을 수 없습니다.");
}
return name;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

자동차 이름에 대한 제약사항은 Cars 객체가 가지고 있는게 맞을까요? 아니면 Car 객체가 가지고 있는게 맞을까요?

Set<String> keys = record.getKeys();
int max = getMax(record);
keys.stream()
.filter(key -> record.getValue(key) == max)
Copy link
Collaborator

Choose a reason for hiding this comment

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

record 에서 값을 가져와서 비교하지 말고,
max 를 인자로 하여 질의하는 메소드를 record 객체에 만들어 보면 어떨까요?

import java.util.List;
import java.util.Set;

public class Winner {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Winner 객체 좋습니다 👍

Comment on lines +6 to +7
private static final int MAX_BOUND = 9;
private static final int MOVABLE_MIN = 4;
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

@findstar findstar left a comment

Choose a reason for hiding this comment

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

수고하셨습니다 👍

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

Successfully merging this pull request may close these issues.

2 participants