Repository navigation
Conversation
|
🤖 AI 리뷰(알고리즘 패턴, 시간/공간 복잡도, 학습 현황) 결과는 Checks 탭에서 볼 수 있어요. |
novacat25
changed the title
|
There was a problem hiding this comment.
안녕하세요 @novacat25 님 :)
지난 온보딩 때도 리뷰를 맡았었는데, 이번에도 맡게 되어 반갑습니다. TS와 점점 가까워지는 중입니다...ㅎㅎ
저도 Python에서 처음엔 DFS(O(n²))로 풀었다가 dict로 O(n)까지 개선했는데, 비슷한 고민의 흔적이 담겨 있어서 반가웠어요.
Map을 Python dict와 비교해 보았습니다.
- map.set(key, value) ↔ d[key] = value
- map.get(key) ↔ d.get(key)
- map.has(key) ↔ key in d
has()로 key가 있는지 확인했더라도 TS는 get()의 반환값을 여전히 number | undefined로 판단하는데, 이를 !로 확정한 이유를 옆에 주석으로 설명해 주셔서 이해하기 쉬웠습니다.
궁금한 점이 하나 있습니다
Q) if 안에서 break로 빠져나가니 별도의 else 없이 그 아래에 map.set(currentNum, i)를 두어도 될 것 같은데 else를 쓰신 이유가 있을까요?
AI 시대일수록 언어를 학습하고 코드로 소통하는 힘이 중요하다고 느끼는데, 서로 다른 언어로 같은 접근을 찾아 나가는 과정이 굉장히 의미 있어서 스터디에 참여하길 정말 잘했다는 생각이 드네요.
이번 주 과제를 정말 빠르게 수행하셨는데, 수고 많으셨습니다 💪
There was a problem hiding this comment.
안녕하세요, 리뷰 감사합니다!
저도 오히려 Python을 다룰 기회가 없어서 Python 메소드로 보니까,
저야말로 관점의 자극이 되어서 기쁩니다!
곰곰이 생각해보니 지적대로네요.
break나 return으로 강제로 빠져나가는 구문이 있으니, else 없어도 되긴 했네요..
else를 쓴 이유는 돌이켜보니 별 건 없고,
약간 조금 더 확실한 안전빵 조건으로 두고 싶지 않았나 하는 마음으로 썼습니다!
실무에서는 확실히 if문만 써서 가독성을 신경쓰긴 했지만,
코딩 테스트만 보면 조금 보수적으로 써서 그럤던 것 같네요.
물론, 조금 더 가독성을 챙긴다면 저 역시 지우는 것에는 찬성합니다..
아무튼, 이번 주도 물론이고 쭉 파이팅 해봐요!
There was a problem hiding this comment.
빠른 피드백 감사합니다🙂
저도 다른 문제 파이썬으로 풀어보고
나머지 문제들도 리뷰 남기겠습니다!🫡
201411108
left a comment
201411108
left a comment
There was a problem hiding this comment.
안녕하세요.
담당 리뷰어는 아니지만 typescript로 풀이를 하신 반가움에 리뷰를 달아봤습니다.
알고리즘과 관련된 부분은 아니고 질문과 가독성과 관련된 내용을 담았어요.
시간 괜찮으실 때 확인해봐주시면 좋을 것 같습니다.
화이팅입니다!
| /* | ||
| 시간 복잡도(Time Complexity): 평균 O(n) | ||
| 공간 복잡도(Space Complexity): 평균 O(n) | ||
| 풀이 알고리즘: JS의 HashMap / Map 이용 |
There was a problem hiding this comment.
❓궁금한 점이 있습니다! 풀이 설명에는 HashMap / Map을 사용하셨다고 적혀 있는데, 실제 코드는 Set을 활용하신 것 같아 질문드립니다.
JS/TS에서 Set과 Map 모두 내부적으로 해시 테이블을 기반으로 동작한다는 점은 같지만, 저장 방식이나 개념은 다른 자료구조로 알고 있어서요 혹시 제가 놓치고 있는 부분이 있거나 다른 의도가 있으셨던 건지 궁금합니다!
There was a problem hiding this comment.
좋은 피드백 감사합니다!
코드를 다시 확인해 보니 말씀해 주신 부분이 맞았습니다.
당시 주석을 작성할 때, Hash Table이라는 내부 탐색 구조와 HashMap / Map이라는 자료 구조의 개념을 혼용했던 것 같습니다.
실제 구현은 Key-Value 쌍을 저장하는 Map이 아니라,
지적하신 대로 중복되지 않는 값들을 저장하고 존재 여부를 확인하는 Set을 사용한 풀이가 맞습니다.
JavaScript의 Map과 Set 모두 해시 테이블과 같은 효율적인 탐색 구조를 활용할 수 있다는 공통점이 있기는 하지만,
엄밀하게는 서로 다른 컬렉션이기 때문에 주석을 JavaScript Set으로 수정하는 것이 정확하겠습니다.
덕분에 자료구조의 개념을 다시 정리할 수 있었습니다.
더 이상 Hash Table, Hash Map, Hash Set 이름으로 헷갈리지 않도록 확실히 짚고 넘어가야 겠네요.
There was a problem hiding this comment.
@201411108 님! 담당 리뷰어가 아니심에도 불구하고 적극적으로 피드백해주셔서 감사합니다:)
정말 상세히 리뷰해주셔서 제가 오히려 도움이 많이 되었습니다
코멘트들이 많이 달려서 저도 참고하여 리뷰 작성하겠습니다.
감사합니다
| for (let i = 0; i < freqArr.length; i++) { | ||
| const [num, freq] = freqArr[i] | ||
| buckets[freq].push(num) | ||
| } |
There was a problem hiding this comment.
💬 for of를 쓰는 방식으로 가독성을 개선해보는 것도 좋을 것 같습니다!
| for (let i = 0; i < freqArr.length; i++) { | |
| const [num, freq] = freqArr[i] | |
| buckets[freq].push(num) | |
| } | |
| for (const [num ,freq] of freqArr) { | |
| buckets[freq].push(num) | |
| } |
There was a problem hiding this comment.
오, 확실히 이 부분도 지적대로십니다.
index를 통한 직접적인 탐색의 이유나 실익이 없으므로, 제시해주신 코드가 훨씬 더 깔끔하네요.
현직 FE 엔지니어 분이셨군요... |
parkhojeong
left a comment
parkhojeong
left a comment
There was a problem hiding this comment.
수고하셨습니다! two-sum 풀이에 대해 리뷰 했습니다~
| if (map.has(anotherPair)) { | ||
| // map.has로 값을 가지고 있음이 확정되어 있으므로 non-null assertion을 통해 값 강제 | ||
| arr[0] = map.get(anotherPair)! | ||
| arr[1] = i | ||
|
|
||
| break | ||
| } |
There was a problem hiding this comment.
has 후 get! 대신 get 결과를 undefined와 비교하면 타입이 number로 좁혀져서 non-null assertion 없이 처리할 수 있을 것 같아요. 조회도 한 번으로 줄어듭니다.
const j = map.get(anotherPair)
if (j !== undefined) return [j, i]There was a problem hiding this comment.
const complementIndex = numToIndexMap.get(complement)
if (complementIndex !== undefined) {
return [complementIndex, i]
}
맞네요. non-null assertion으로 위험한 타입 강제를 하지 않고도 깔끔하게 바뀌었습니다.
has로 검증이 되면 get으로 undefined를 좁히지 않을 줄 알았는데, 아니었더라고요..
| */ | ||
|
|
||
| const twoSum = (nums: number[], target: number): number[] => { | ||
| const map: Map<number, number> = new Map() |
There was a problem hiding this comment.
변수명만 보고 어떤 값인지 알기 어려운데, 적절한 네이밍을 해보시는건 어떨까요?
There was a problem hiding this comment.
const numToIndexMap: Map<number, number> = new Map()
확실히 단순 map이면 위험했습니다.
변수명 의미도 살지 않고 해서 numToIndex 식으로 Key/value 관계가 잘 살도록 수정했습니다.
더불어 return OOO 부분에서도 임의값을 주지 않고
throw new Error로써 조건에 맞지 않는 문제임을 확실히 해서 예외처리를 넣었습니다.
|
|
||
| 풀이 History | ||
| 1트: 완전 탐색을 이용해 정답은 맞췄으나, Time Complexity가 O(n²) | ||
| 2트: HasyMap을 통해 Key - value 플로를 응용하여 O(n)으로 단축 완료 |
There was a problem hiding this comment.
위 코멘트에 이어서 답변 드렸습니다!
JavaScript Map을 활용하여 숫자와 인덱스를 Key-Value 형태로 관리.
| /* | ||
| 시간 복잡도(Time Complexity): 평균 O(n) | ||
| 공간 복잡도(Space Complexity): 평균 O(n) | ||
| 풀이 알고리즘: JS의 HashMap / Map 이용 |
There was a problem hiding this comment.
nit: JS에는 HashMap 클래스가 없으니 contains-duplicate처럼 Hash Map (JavaScript Map)으로 맞추면 어떨까요? HashMap / Map은 둘 중 하나를 썼다는 뜻으로 읽힐 여지가 있어 보여요.
There was a problem hiding this comment.
소중한 피드백 감사합니다!
아래처럼 바꿨습니다.
JavaScript Map을 활용하여
숫자와 인덱스를 Key-Value 형태로 관리.
보완해야 할 숫자의 존재 여부를 평균 O(1)에 탐색하여
전체 시간 복잡도를 O(n)으로 개선.
아까 리뷰에서 복습한 것처럼... Hash Map 쪽 개념을 확실히 할 필요가 있네요.
코멘트 부분은 아예 일괄수정을 했는데...
The specification requires maps to be implemented "that, on average, provide access times that are sublinear on the number of elements in the collection". Therefore, it could be represented internally as a hash table (with O(1) lookup), a search tree (with O(log(N)) lookup), or any other data structure, as long as the complexity is better than O(N).
를 보면 JavaScript에는 HashMap, HashSet이라는 이름의 표준 내장 클래스가 없고
Map, Set의 내부 구현이 반드시 Hash Table이어야 한다고 ECMAScript 명세가 강제하지 않아서 조금 더 표현을 다듬었습니다.
답안 제출 문제
작성자 체크 리스트
In Review로 설정해주세요.검토자 체크 리스트
Important
본인 답안 제출 뿐만 아니라 다른 분 PR 하나 이상을 반드시 검토를 해주셔야 합니다!