Skip to content

Conversation

@chyo1
Copy link

@chyo1 chyo1 commented Jan 9, 2024

No description provided.

Copy link
Contributor

@Ssuamje Ssuamje left a comment

Choose a reason for hiding this comment

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

LV 1 : 각 턴마다 출력 간 개행이 추가되면 요구사항을 더 잘 만족할 것 같습니다! (지금은 붙어서 출력 됨)

변수명이나 상수 관리 부분에서 조금 더 신경 써 주시면 더 이쁜 코드로 금방 작성하실 것 같습니다!
자바 금방 하시네요! 고생하셨습니다👍👍

Comment on lines +17 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

  • 매개변수를 재할당하는 것 보다는 새로운 값을 지정해서 사용하는 편이 좋을 것 같습니다!
    이후의 흐름에서 order라는 매개변수가 원본 값임을 기대하는데 중간에 사이드 이펙트(부작용, 값이 바뀌는 행위)가 생기면 이후 다른 사람이 흐름을 파악하기가 어려울 수 있다고 생각해요.

  • indexOf()와 substring()을 이용하는 부분은 '주어지는 주문 입력에 대해 주문은 이렇게 분리된다'라는 로직을 적용하는 부분인데, 해당하는 로직을 갖는 객체(우리가 정의한 '주문(Order)')로 정의해서 사용하면 좀 더 객체지향적일 것 같습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

  • 아직 객체가 어색하다면, 하나의 기능을 하는 함수를 빼보는 것부터 시작해도 좋습니다!!

  • 추가적으로, 이부분도 사람마다 다르지만 저는 가독성을 생각한다면 변수명은 상세할 수록 좋다고 생각합니다! loc보다는 prefixPosition 혹은 prefixIndex같은 변수명은 어떨까여

  • 위에 언급드린바와 같이, 파싱의 대상인 prefix, suffix(<, >)는 위에 static으로 선언하는 방식도 고려해보는게 좋을 것 같습니다! 만약 <,>를 여기저기 많이 써놨는데, 이제부턴 {,}로 파싱해주세요 혹은 >,<로 파싱해주세요~ 라고 하면 일일이 찾아서 고쳐야하니까엽

Copy link
Contributor

Choose a reason for hiding this comment

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

Map 등의 Collection류 변수명은 복수형으로 작성해주시면 좋습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

'I'와 같은 표현은 주로 사용하지 않습니다..!
손님이 '시켰나?' 라고 물어본 것에 대해서 'name'이라는 변수에 해당하는 아이템이 있는지 찾는 것이므로 �didProductOrdered와 같은 방식으로 바꿀 수 있을 것 같습니다!

  • 해당 메서드를 호출하는 객체는 주문을 받는 Bot이어서 order를 하는 주체가 아닌 점도 있는 것 같아요!

Copy link
Contributor

Choose a reason for hiding this comment

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

메서드명은 맨 앞에 동사가 오게끔 작성해주세요!

Copy link
Contributor

Choose a reason for hiding this comment

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

'1000'과 같은 부분은 정책적으로 변경될 여지가 있는 부분인데, 해당하는 부분의 변경에 대해서 상수 값으로 관리하지 않으면 이후에 변경된 시점에 일일히 바꿔주어야 합니다..!

정책적으로 지정된 값들에 대해서는 별도의 상수(static final 혹은 enum)로 관리해주시는게 좋을 것 같습니다!

Comment on lines +24 to +28
Copy link
Contributor

Choose a reason for hiding this comment

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

today1, today2 라는 변수명이 나오게 된 경위가 '풀 수 있는 알고리즘 전체의 이름을 가진 리스트'를 기준으로 '해당하는 리스트의 원소 개수가 끝 값인 난수'를 지정한 후에 '인덱스로 지정해서 두 가지를 뽑아낸다'로 이해했는데,

today1, today2라는 이름은 직관적으로 다른 사람이 이해하기 어려울 수 있으므로

Algorithm todayAlgorithm1 = algorithm.get((int) ((Math.random() * 10) % 5));
Algorithm todayAlgorithm2 = algorithm.get((int) ((Math.random() * 10) % 5));

라는 방식으로 표현할 수 있을 것 같습니다.
좀 더 낫게 만든다면 (int) ((Math.random() * 10) % 5)에 해당하는 부분을 public int getRandomIntByRange(int from, int to)와 같은 방식으로 메서드를 분리해서 다음과 같이 표현해준다면 더 읽기 좋을 것 같아요!

Algorithm todayAlgorithm1 = algorithm.get(getRandomIntByRange(1, 5));

Comment on lines +30 to +31
Copy link
Contributor

Choose a reason for hiding this comment

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

해당 부분은 todayAlgorithm1, 2를 대입해서 사용할 수 있을 것 같은데 아마도 깜빡하신 부분인 것 같습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

매개변수 명이 haveToSolve인 것 보다는 Algorithm algorithm으로 적어주시는 것이 가독성에 더 좋을 것 같습니다! (해당 메서드로 들어온 이상 solve를 진행하는 것은 보장되어 있기 때문에)

Comment on lines +12 to +13
Copy link
Contributor

Choose a reason for hiding this comment

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

람다식을 사용하실 때 사용되는 변수에 대해서는 변수명을 명기해주시는 것이 좋습니다!
위의 케이스에서는 x라고 적혀있으면 다른 사람이 읽을 때 해당하는 변수가 무엇에 해당하는지 찾아보아야 하는데요,
'algorithm ->' 혹은 'algo ->', 'a ->' 라는 식으로 적어주시면 가늠이 가능합니다!

Optional<Algorithm> today1 = algorithmMap.keySet().stream()
				.filter(algorithm  -> algorithm.isSolution(haveToSolve))
				.findFirst();

Copy link
Contributor

Choose a reason for hiding this comment

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

람다식 마스터 hyowchoi?
이거 못막습니다.. 변수명만 조금 더 신경써봅시더!!

Copy link
Contributor

Choose a reason for hiding this comment

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

스네이크 케이스 -> 카멜 케이스 주의해주세요!

Copy link
Contributor

@saewoo1 saewoo1 left a comment

Choose a reason for hiding this comment

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

자바 처음해보셨다는데 너무너무 잘 작성하신 것 같습니다!!!!
제가 자바를 처음 시작했을 때 했던 고민들이 많이 녹아나있는게 보였고, 개인적으로 공부하면서 고민했던 부분들에 대해서 모두 적어봤습니다. 앞으로 함께 열심히 해봐여!!

당신? 재능있어

Copy link
Contributor

Choose a reason for hiding this comment

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

자바는 보통 변수명을 카멜케이스로 작성합니다!
nowAnger, angerThreshold 이런 식으로여!

Copy link
Contributor

Choose a reason for hiding this comment

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

40, 80, 10000과 같이 직접 수를 작성하기보단, 변수명을 주는건 어떨까요?
다른 사람이 코드를 읽었을 때, 40, 80의 의미가 뭔지 바로 알 수 있도록여!

Copy link
Contributor

Choose a reason for hiding this comment

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

생성자는 뭐하는 애고, 어떻게 써야할까? 에 대해 고민해봅시다!!!
이후에 책임 분리까지 관심이 생기신다면 정적 팩토리 메서드도 한숟가락..

https://sun-22.tistory.com/84

https://tecoble.techcourse.co.kr/post/2020-05-26-static-factory-method/

Copy link
Contributor

Choose a reason for hiding this comment

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

클래스 내부에 property(필드)를 갖고 있다면, 해당 필드를 갖고 뭘 할 수 있을까? 얘를 갖고 어떤 기능을 넣고, 이용할 수 있을까? 에 대한 고민을 하면 정말 조습니다.
할 수 있는게 없다면, 진짜 필요한 필드일까?에 대해 고민해보아여

Copy link
Contributor

Choose a reason for hiding this comment

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

각자의 코딩 스타일이 있겠지만, 무한루프보다는 탈출조건까지만 함수를 시행하는건 어떨까요??
탈출 조건 명시도 잘 해두셨으니까여!!

Copy link
Contributor

Choose a reason for hiding this comment

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

Map에서 get을 쓴다는 것.. NullPointException을 감내한다는 것.. input에 대한 예외사항을 견딜 자신이 있다는 것..!!
과제를 진행하면서 개인적으로 고민을 많이 했던 부분인데, 예외사항에 대해서 추가적인 처리를 도입하거나, getOrDefault로 우회하는 방식을 생각해보면 좋을 것 같습니다!! 함께 더 고민해봅시더..

Copy link
Contributor

Choose a reason for hiding this comment

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

책임 분리란 무엇일까여.. 항상 고민되는 부분입니다.
핵심 로직인 값 계산과 출력이 함께 얽혀있는데, 어떻게 둘을 떼어낼 수 있을까요?!

Comment on lines +10 to +17
Copy link
Contributor

Choose a reason for hiding this comment

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

언급해주신바와 같이 JDK 17의 좋은 업데이트 Collections 대신 List.of로 축약하는 기능을 사용하신다면 당신은 자바 멋쟁이

Copy link
Contributor

Choose a reason for hiding this comment

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

저와 함께 오늘 이부분 리팩토링 해볼까요!!!! 좋은 공부가 될 것 같습니다

Comment on lines +12 to +13
Copy link
Contributor

Choose a reason for hiding this comment

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

람다식 마스터 hyowchoi?
이거 못막습니다.. 변수명만 조금 더 신경써봅시더!!

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.

4 participants