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

[Hamill] 코드 리뷰 요청 #49

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

[Hamill] 코드 리뷰 요청 #49

wants to merge 6 commits into from

Conversation

hanurii
Copy link
Contributor

@hanurii hanurii commented Sep 7, 2020

처음부터 정렬까지 구현했습니다

  • 기본 자료구조 구현
  • 미션 구현

@hanurii hanurii requested a review from ksundong September 7, 2020 13:55
@hanurii hanurii self-assigned this Sep 7, 2020
Copy link
Member

@ksundong ksundong left a comment

Choose a reason for hiding this comment

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

밥먹고 또 리뷰하겠습니다.

몇 몇 변경이 필요한 부분이 있어서 change request 드려요 ㅎㅎ


private static int binarySearch(int[] numbers, int number, int first, int last) {
while (first <= last) {
int mid = (first + last) / 2;
Copy link
Member

Choose a reason for hiding this comment

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

이 부분은 int overflow를 조심하셔야해요.
https://endorphin0710.tistory.com/112
여길 참고해보세요.

public class BinarySearchOperationCount {

private static int binarySearch(int[] numbers, int number, int first, int last) {
int mid;
Copy link
Member

Choose a reason for hiding this comment

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

이건 약간 윤성우님 책 스타일로 선언을 했네요 ㅋㅋ
저라면 초기화와 동시에 선언하는 스타일을 사용했을 것 같습니다.

int opCount = 0;

while (first <= last) {
mid = (first + last) / 2;
Copy link
Member

Choose a reason for hiding this comment

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

여기도 리뷰 참고해서 수정하시길 바랄게요!


private static int binarySearch(int[] numbers, int number, int first, int last) {
int mid;
int opCount = 0;
Copy link
Member

Choose a reason for hiding this comment

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

제가 윤성우 책에서 안좋아하는 부분이 명확하지 않은 변수명이에요.
약어를 사용하시기 보다는 보다 명확한 변수명을 사용하시는게 좋을 것 같아요.

Comment on lines +6 to +7
int i;
for (i = 0; i < numbers.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

앗... 아아...

Comment on lines +35 to +37
public int count() {
return this.numOfData;
}
Copy link
Member

Choose a reason for hiding this comment

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

메서드명은 관례상 size()가 좀 더 적합해보입니다.

Comment on lines +39 to +41
public Integer get(int index) {
return this.numbers[index];
}
Copy link
Member

Choose a reason for hiding this comment

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

wrapper type으로 반환하게 되면 auto boxing이 일어나기 때문에
여기선 불필요하다고 보입니다.
Integerint 타입은 많은 차이가 있습니다.
특히 메모리 공간, Nullable 등의 차이가 있겠네요.

Comment on lines +43 to +52
public Integer remove(int index) {
Integer data = this.numbers[index];

for (int i = index; i < this.numOfData - 1; i++) {
this.numbers[i] = this.numbers[i+1];
}

this.numOfData--;
return data;
}
Copy link
Member

Choose a reason for hiding this comment

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

데이터가 없다면 0을 주겠네요?
뭔가 이상하지 않을까요 ㅎㅎ

Integer data = this.numbers[index];

for (int i = index; i < this.numOfData - 1; i++) {
this.numbers[i] = this.numbers[i+1];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.numbers[i] = this.numbers[i+1];
this.numbers[i] = this.numbers[i + 1];

Comment on lines +54 to +56
public int size() {
return this.numbers.length;
}
Copy link
Member

Choose a reason for hiding this comment

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

응? 이거 어떻게보면 맞는 것 같기도 하고 아닌 것 같기도 합니다.
java의 size는 어떻게 동작하나요?

Copy link
Member

@ksundong ksundong left a comment

Choose a reason for hiding this comment

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

  1. 의미있는 변수명 사용하기
  2. if문으로 예외를 처리할 때, 출력만 하지말고, 종료 포인트 잡아주기
  3. 생성자 생성시점에 초기화되는 값은 굳이 불필요하게 안적어도 되면 적지말기
  4. 자바 컨벤션 지켜주기

이 밖에도 많은 코멘트가 있습니다.
참고해서 수정해주세요 ㅎㅎ...

Comment on lines +16 to +26
// int indexOf(T o);
// int lastIndexOf(T o);

// ListIterator listIterator();
// ListIterator listIterator(int index);

// T set(int index, T element);

// void sort(Comparator c);

// List subList(int fromIndex, int toIndex);
Copy link
Member

Choose a reason for hiding this comment

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

ㅋㅋㅋ 나중에 구현할 사항들인가요?

public static void main(String[] args) {
List<Integer> list = new ArrayList<>();
for (int i = 0; i < 9; i++) {
list.add(i,i+1);
Copy link
Member

Choose a reason for hiding this comment

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

전반적으로 java coding convention이 잘 지켜지지 않는 모습이네요.

Comment on lines +30 to +33
Iterator<Integer> iterator = list1.iterator();
while (iterator.hasNext()) {
System.out.println(iterator.next());
}
Copy link
Member

Choose a reason for hiding this comment

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

enhanced for loop를 사용해도 됩니다.

Comment on lines +3 to +6
public class NameCard {

private String[] name;
private String[] phone;
Copy link
Member

Choose a reason for hiding this comment

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

좀 더 객체지향적인 접근을 해보세요 ㅎㅎ

Copy link
Member

Choose a reason for hiding this comment

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

요 코드는 다시 작성하는게 좋아보입니다.
전체적으로 자바로 짠 느낌이 하나도 들지 않았어요.

Comment on lines +5 to +9
// 모든 멤버변수는 public static final. 생략 가능
int coin100Num = 0;
int bill5000Num = 0;

// 모든 메서드는 public abstract 이어야함. 생략 가능
Copy link
Member

Choose a reason for hiding this comment

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

학습 내용은 좋아보입니다.
근데 보통 interface에서는 멤버변수 선언을 잘 안하는 편이고, 네이밍 컨벤션에 맞게 작성해주시는게 좋을 것 같아요.

for (int i = 0; i < afterNum + count; i++) {
if (i != afterNum + count - 1) {
searchNode = searchNode.next;
continue;
Copy link
Member

Choose a reason for hiding this comment

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

continue는 필요 없습니다.

@@ -0,0 +1,74 @@
package com.codesquad.datastructurestudy.list.linkedlist.yoon;

public class DDoublyLinkedList {
Copy link
Member

Choose a reason for hiding this comment

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

👀

Comment on lines +11 to +23
public DDoublyLinkedList() {
this.dummyNodeHead = new Node(-1);
this.dummyNodeTail = new Node(-2);

this.dummyNodeHead.prev = null;
this.dummyNodeHead.next = this.dummyNodeTail;

this.dummyNodeTail.prev = dummyNodeHead;
this.dummyNodeTail.next = null;

this.head = this.dummyNodeHead;
this.tail = this.dummyNodeTail;
}
Copy link
Member

Choose a reason for hiding this comment

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

생성자가 너무 웅장합니다...

Copy link
Member

Choose a reason for hiding this comment

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

굳이 dummyNode 부분을 분리해야 했을까요?

Comment on lines +39 to +41
if (tail.prev == head) {
System.out.println("삭제할 노드가 없습니다");
}
Copy link
Member

Choose a reason for hiding this comment

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

출력 이후에 아래의 로직이 실행됩니다~

Comment on lines +52 to +54
public static void main(String[] args) {

}
Copy link
Member

Choose a reason for hiding this comment

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

0ㅇ0

- 힙은 완전이진트리 형태이며, 값에 우선순위에 따라 최대힙, 최소힙으로 불린다.
  - 작은 값이 우선순위가 높으면 최소힙, 큰 값이 우선순위가 높으면 최대힙이라고 한다.
- 우선순위 큐는 들어갈 때는 큐와 다를바 없이 들어가지만 나올 때는 우선순위가 가장 높은 것이 나오는 자료구조다.
  - 배열, 연결리스트, 힙으로 구현할 수 있으며 각각 장단점이 있지만 힙으로 구현했을 때가 가장 우선순위 큐 구현에 적절하다.
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.

2 participants