Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[BE] refactor: 하이라이트 매핑 로직 간소화 #962
[BE] refactor: 하이라이트 매핑 로직 간소화 #962
Changes from all commits
b8dbf31
5c71f7b
52d417c
c86f835
81050dc
7d0fec3
b151f1e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이해가 자연스러운 흐름은
higlightLines가 하이라이트가 쳐져있는 line만 갖고 있고, 그걸 엔티티로 변환한다.인데
현재는
HighlightedLines
의 필드 lines는 하이라이트가 안쳐져있는 line까지 포함하고 있지만, 엔티티로 변환할 때는 하이라이트가 쳐져있는 부분만 걸러서 변환이 된다.의 로직이라 조금더 이해하는데 시간이 걸리네요.(네 맞습니다. 제가 생성해놓은 구조..ㅎㅎ)
그렇다면 왜 하이라이트가 안쳐져있는 라인까지 포함해야했나 보면 이런 이유때문이었네요.
1번의 흐름으로 코드를 구성하는 방법이 있을지 고민입니다...!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(지금 변경하자는 논의는 아니고, 함께 고민해보자입니다~)
위의 코멘트와 별개로 이곳이 연산이 많이 일어나는 포인트겠구나 싶어요. 하이라이트의 대상인 라인만큼 스트림 연산이 일어날테니까요. 새삼 json으로 저장했다면 이런 연산 비용은 적었겠다는 생각이 들었습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 이것도 엄청 이해가 어려운 건 아니에요~ 오잉 싶었던 부분 짚고 넘어가고 싶어서!
제일 땡겼던 건: 1번으로 구조 자체가 바뀐다면 이해하기도 좋고 네이밍 문제도 해결되겠다
근데: 지금 당장 바꿀정도는 아니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는
스트림의 키가 뭐더라... value가.. 여기서 flatMap을 하면...
처럼 생각이 어려워져서, for문이 더 길더라도 이해가 쉽네요😂There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stream
과entry
에 익숙해지는 것이 좋을까요, 풀어두는 게 좋을까요? 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
answerIdHighlightedLines
가 이미 맵의 형태 네이밍이 되어 있으니entrySet
을 했을 때의 결과가 예측되려나요?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네이밍을 통해 key-value가 뭐일지 예측이 되는 건 맞네요!
뭔가 계속 읽다보니 스트림도 충분히 이해 가는 것 같기도하고...ㅋㅋㅋㅋ
흠.. 다들 스트림 방식을 더 선호한다면 크게 이해가 어려운 것은 아니니 스트림으로 가도 좋습니다!