대소문자를 구분하지 않는 Attributes #75

sanhee · 2021-12-08T16:53:26Z

close #67

대소문자를 구분하지 않는 Attributes 를 구현하기 위해
처음 시도했던 방법은 들어온 헤더 필드를 강제로 대문자로 변경 후 저장하는 구조였습니다.
다만, 기존 테스트 케이스가 깨지고, 예상값도 대문자로 해줘야 하는 불편함이 있어 개선이 필요했습니다.

따라서, Attribute의 맵에 값을 추가할 때
대소문자 관계없이 해당 원소가 존재하는지 맵을 순차적으로 비교하고
없을 경우 추가하는 방식으로 로직을 수정했습니다.

삽입, 조회시 강제로 대문자로 수정해서, 대소문자를 구분하지 않는 로직을 구현했습니다.

#67 기존 들어온 헤더 필드를 강제로 대문자로 변경 후 저장하는 구조였는데, 기존 테스트 케이스가 깨지고, 예상값도 대문자로 해줘야 하는 불편함이 있었습니다. 따라서, Attribute의 맵에 값을 추가할 때 대소문자 관계없이 해당 원소가 존재하는지 맵을 순차적 O(n)으로 비교하고 없을 경우 추가하는 방식으로 로직을 수정했습니다.

sanhee · 2021-12-08T16:58:10Z

src/main/java/webserver/http/attribute/Attributes.java

+                return attributes.get(k);
+            }
+        }
+        return null;


null이 조금 걸리네요..ㅎㅎ

exception
null을 받아야할 이유가 있다면: optional

getDefault가 있는데 굳이 null을 리턴해줄 필요는 없다고 보여집니다.
꼭 써야한다면 Optional로 제공해주는 것도 고려해볼 수 있을 것 같아요

그리고 null을 명시적으로 리턴해준다면 get이 null인 케이스도 테스트에서 확인을 해봐야할 것 같습니다!

이부분 작성하면서도 이상하다 생각했는데 감사합니다!

Dae-Hwa

고생 많으셨어요
생각하기 어려운 로직인데 잘 처리된 것 같네요

src/main/java/webserver/http/attribute/Attributes.java

Dae-Hwa · 2021-12-09T01:22:40Z

src/main/java/webserver/http/attribute/Attributes.java

    }

    public Attributes add(String key, String value) {
+        for (String k : attributes.keySet()) {


직접 사용할 변수면 명시적으로 이름을 짓는 것도 좋아보여요
eachKey currentKey 같은거라도 좋을 것 같고
그게 힘들면 매개변수 이름을 targetKey 같은 걸로 나눠줘도 좋을 것 같습니다.

앗.. 너무 막지었네요.
지적 감사합니다.

Dae-Hwa · 2021-12-09T01:23:24Z

src/main/java/webserver/http/attribute/Attributes.java

+                return attributes.get(k);
+            }
+        }
+        return null;


getDefault가 있는데 굳이 null을 리턴해줄 필요는 없다고 보여집니다.
꼭 써야한다면 Optional로 제공해주는 것도 고려해볼 수 있을 것 같아요

Dae-Hwa · 2021-12-09T01:24:56Z

src/main/java/webserver/http/attribute/Attributes.java

+        for (String k : attributes.keySet()) {
+            if (k.equalsIgnoreCase(key)) {
+                return attributes.get(k);
+            }
+        }
+        return defaultValue;


코드 중복이 되는데 get이나 getOrDefault 둘 중 하나를 기준으로 중복 제거를 해볼 수 있겠네요

Dae-Hwa · 2021-12-09T01:25:31Z

src/main/java/webserver/http/statusline/RequestStatusLine.java

+        Attributes statusLineAttributes = new Attributes();

-        statusLineAttributes.put(METHOD_KEY, statusLine.get(0));
-        statusLineAttributes.put(PATH_KEY, statusLine.get(1));
-        statusLineAttributes.put(PROTOCOL_VERSION_KEY, statusLine.get(2));
+        statusLineAttributes.add(METHOD_KEY, statusLine.get(0));
+        statusLineAttributes.add(PATH_KEY, statusLine.get(1));
+        statusLineAttributes.add(PROTOCOL_VERSION_KEY, statusLine.get(2));


이 부분도 Attributes를 만들면서 자연스럽게 검증이 될 테니 Map으로 받아줘도 괜찮을 것 같아요

그게 아니라 생성자로 Attributes를 넣어주는게 더 바람직하다고 생각하신거면 좋은 방법인 것 같습니다
만약 그렇다면 Map<String, String>을 받는 생성자를 완전히 대체해주는게 좋을 것 같습니다.

Dae-Hwa · 2021-12-09T01:29:19Z

src/test/java/webserver/http/attribute/AttributesTest.java

+        assertAll(
+                () -> assertEquals("value", attributes.get("KEY")),
+                () -> assertEquals("value", attributes.get("key")),
+                () -> assertEquals("value", attributes.get("kEy"))
+        );


한 번에 여러개의 케이스를 테스트 하는 것 보다는 매개변수화 시키는게 더 좋을 것 같습니다

혹시 매개변수화 시켜서 테스트를 하면 어떤 장점이 있는건가요??
아직 정확하게 와닿지 못해서 이런 방식으로 작성했습니다!

기본적으로 저는 assertAll은 같은 테스트에 대한 그룹을 묶어주는 것이라 생각했습니다. 예를 들어 검증해야 될 객체에 여러개의 필드가 들어있고 각각 검증을 해야 하는 상황이라면 묶어주는게 의미가 있을거라 생각합니다.
하지만, assertAll로 여러개의 테스트 케이스를 묶어주면 여러개의 테스트가 하나의 테스트 안에 들어가있는 느낌이라 좋지 못하다고 생각한 것 같습니다. 기본적으로 하나의 테스트는 한 가지 테스트를 수행하는게 가장 좋다고 알고 있기 때문입니다.
마지막으로 가독성에 대한건 개인 취향이긴 한데, RequestHandlerTest에 있는 케이스들을 저런 식으로 처리한다면 가독성이 매우 떨어질 것 같습니다.

Dae-Hwa · 2021-12-09T01:29:39Z

src/test/java/webserver/http/attribute/AttributesTest.java

+                () -> assertEquals(expectedAttributes.get("key"), attributes.get("KEY")),
+                () -> assertEquals(expectedAttributes.get("KEY"), attributes.get("kEY")),
+                () -> assertEquals(expectedAttributes.get("kEy"), attributes.get("key"))


키가 중복돼서 들어가지 않는 케이스를 추가해주는게 좋을 것 같습니다

size같은걸로 검증해볼 수 있을 것 같네요

예외상황 추가해주셔서 감사합니다!

Dae-Hwa · 2021-12-09T01:31:04Z

src/main/java/webserver/http/attribute/Attributes.java

+                return attributes.get(k);
+            }
+        }
+        return null;


그리고 null을 명시적으로 리턴해준다면 get이 null인 케이스도 테스트에서 확인을 해봐야할 것 같습니다!

Dae-Hwa · 2021-12-09T01:33:33Z

추가적으로 Attributes의 Map을 계속 탐색하는 대신에 Key를 타입으로 분리해볼 수 있을 것 같습니다.
Map<Key, String> 과 같이 만들 수 있을 것 같아요. Key 내부에서 올바른지 판단을 하니 책임소재도 명확해지고 불필요한 전체 순회도 없어질거라 생각합니다.
물론 Attributes가 성능에 영향을 줄 정도로 많은 일은 드물겠지만, 개선해볼 수 있을 것 같아요

이런식으로 분리하다보면 나중에 Value 파트도 스프링의 MultiValueMap처럼 리스트 타입으로 해야 하거나 하면 쉽게 분리할 수 있을 것 같아요

# Conflicts: # src/main/java/webserver/http/statusline/StatusLine.java

# Conflicts: # src/main/java/webserver/http/startline/RequestLine.java # src/test/java/webserver/http/header/RequestHeaderTest.java # src/test/java/webserver/http/header/ResponseHeaderTest.java # src/test/java/webserver/http/startline/RequestLineTest.java # src/test/java/webserver/http/startline/StatusLineTest.java

Dae-Hwa

고생하셨습니다
아쉬운 부분이 있어서요 한 번만 얘기 더 해봐요 ㅎㅎ

Dae-Hwa · 2021-12-21T01:15:18Z

src/main/java/webserver/http/attribute/Attributes.java

+        try {
+            return get(targetKey);
+        } catch (IllegalArgumentException e) {
+            return defaultValue;
+        }


try-catch를 안 쓰는 방법도 있지 않을까요??

Dae-Hwa · 2021-12-21T01:16:36Z

src/main/java/webserver/http/attribute/Attributes.java

+        for (String currentKey : attributes.keySet()) {
+            if (currentKey.equalsIgnoreCase(targetKey)) {
+                return attributes.get(currentKey);
+            }
+        }
+        throw new IllegalArgumentException("일치하는 키가 없습니다.");


key에 해당하는 value가 null이라면 제대로 동작하지 않을 것 같아요
이런 경우를 막지 않아도 괜찮을까요?
그리고 예외를 발생시키면서까지 get을 사용해야할 경우가 있을까요?
getOrDefault로는 안되는 경우가 있는건지 궁금합니다

key: "null", value: null

인 상태에서 key값으로 "null"을 꺼내면 null을 반환 하는데
제대로 동작하지 않는다 게 어떤 부분인지 잘 모르겠습니다.

f61aa7f 에서 try-catch를 제거하는 방식으로 수정했습니다.

혹시 생각하신 방식이랑 다르다면 말씀해주세요!

src/main/java/webserver/http/attribute/Attributes.java

Dae-Hwa · 2021-12-21T01:23:36Z

src/main/java/webserver/http/attribute/Attributes.java

-    public Attributes add(String key, String value) {
-        attributes.put(key, value);
+    public Attributes add(String targetKey, String value) {
+        for (String currentKey : attributes.keySet()) {


👍👍👍👍

sanhee · 2021-12-22T03:29:47Z

@Dae-Hwa

아까 얘기 나눈 거 반영했습니다!

8dbb272
41592da

assertAll은 동일한 케이스에서 검사해야 하는 로직이라고 생각해 사용했습니다!

Dae-Hwa · 2021-12-22T15:45:59Z

src/test/java/webserver/http/attribute/AttributesTest.java

+                ()-> assertThat(attributes.get("null")).isNull(),
+                ()-> assertThat(attributes.get("null2")).isNull()


둘 다 null을 확인하는 별개의 테스트케이스 같은데, 따로 묶은 이유가 있으신가요?

Dae-Hwa · 2021-12-22T15:46:35Z

src/test/java/webserver/http/attribute/AttributesTest.java

+        attributes.add("null2", null);
+
+        assertAll(
+                ()-> assertThat(attributes.size()).isEqualTo(2),


null을 확인하는 테스트 같은데 사이즈를 확인하는 이유가 어떤건가요?

sanhee added 4 commits December 6, 2021 10:34

feat: 대소문자 구분하지 않는 Attributes

5106c52

삽입, 조회시 강제로 대문자로 수정해서, 대소문자를 구분하지 않는 로직을 구현했습니다.

Merge branch 'freddie-noel' into feat/attributes/case-insensitive

532ad2e

Merge branch 'freddie-noel' into feat/attributes/case-insensitive

c698959

sanhee added the 미션 label Dec 8, 2021

sanhee requested a review from Dae-Hwa December 8, 2021 16:53

sanhee self-assigned this Dec 8, 2021

sanhee commented Dec 8, 2021

View reviewed changes

Dae-Hwa requested changes Dec 9, 2021

View reviewed changes

sanhee added 8 commits December 10, 2021 11:07

feat: Attributes.get() 없는 키의 경우 예외처리 추가

0d3fa81

refactor: Attributes의 반복문에서 사용중인 변수명 명시적으로 정의

b0ee9ae

refactor: RequestStatusLine 생성자 파라미터 Attribute만 가능하도록 변경

0c06f60

refactor: AttributeTest의 add, addAll 테스트 매개변수화

780f6e9

Merge branch 'freddie-noel' into feat/attributes/case-insensitive

2851f2a

# Conflicts: # src/main/java/webserver/http/statusline/StatusLine.java

refactor: ResponseStatusLine 생성자 파라미터 Attribute만 가능하도록 변경

dfe49f7

fix: StatusLine으로 이름 변경되서 생긴 충돌 해결

66dcf7c

sanhee mentioned this pull request Dec 20, 2021

Attributes의 Map을 계속 탐색하는 대신에 Key를 타입으로 분리 #80

Open

sanhee requested a review from Dae-Hwa December 20, 2021 02:16

Dae-Hwa requested changes Dec 21, 2021

View reviewed changes

Dae-Hwa assigned Dae-Hwa and unassigned sanhee Dec 21, 2021

refactor: Attributes get() 메소드 예외발생 로직 제거

f61aa7f

sanhee requested a review from Dae-Hwa December 22, 2021 01:57

sanhee added 2 commits December 22, 2021 12:15

refactor: Attributes getOrDefault() 리턴문 인라인으로 정리

8dbb272

test: Attributes value가 null인 key

41592da

Dae-Hwa requested changes Dec 22, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

대소문자를 구분하지 않는 Attributes #75

대소문자를 구분하지 않는 Attributes #75

sanhee commented Dec 8, 2021 •

edited by Dae-Hwa

Loading

sanhee Dec 8, 2021

sanhee Dec 9, 2021

Dae-Hwa Dec 9, 2021

Dae-Hwa Dec 9, 2021

sanhee Dec 10, 2021 •

edited

Loading

Dae-Hwa left a comment

Dae-Hwa Dec 9, 2021

sanhee Dec 10, 2021

Dae-Hwa Dec 9, 2021

Dae-Hwa Dec 9, 2021

Dae-Hwa Dec 9, 2021

Dae-Hwa Dec 9, 2021

Dae-Hwa Dec 9, 2021

sanhee Dec 10, 2021

Dae-Hwa Dec 10, 2021

Dae-Hwa Dec 9, 2021

Dae-Hwa Dec 9, 2021

sanhee Dec 10, 2021

Dae-Hwa Dec 9, 2021

Dae-Hwa commented Dec 9, 2021

Dae-Hwa left a comment

Dae-Hwa Dec 21, 2021

Dae-Hwa Dec 21, 2021

sanhee Dec 22, 2021

sanhee Dec 22, 2021 •

edited

Loading

Dae-Hwa Dec 21, 2021

sanhee commented Dec 22, 2021 •

edited

Loading

Dae-Hwa Dec 22, 2021

Dae-Hwa Dec 22, 2021

		()-> assertThat(attributes.get("null")).isNull(),
		()-> assertThat(attributes.get("null2")).isNull()

대소문자를 구분하지 않는 Attributes #75

Are you sure you want to change the base?

대소문자를 구분하지 않는 Attributes #75

Conversation

sanhee commented Dec 8, 2021 • edited by Dae-Hwa Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanhee Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

Dae-Hwa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dae-Hwa commented Dec 9, 2021

Dae-Hwa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanhee Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanhee commented Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanhee commented Dec 8, 2021 •

edited by Dae-Hwa

Loading

sanhee Dec 10, 2021 •

edited

Loading

sanhee Dec 22, 2021 •

edited

Loading

sanhee commented Dec 22, 2021 •

edited

Loading