Yasmin/url parser #18
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vvvpiano
reviewed
Jan 29, 2021
|
|
||
| constructor(url) { | ||
| this.#url = url; | ||
| this.#full_path = url.split('://')[1].split(':')[1].split('/'); |
There was a problem hiding this comment.
Uri에 포트번호가 없는 경우에는 두번째 스플릿에서 outofboundindex exception이 뜨지 않을까하는 생각이 듭니당...!
| constructor(url) { | ||
| this.#url = url; | ||
| this.#full_path = url.split('://')[1].split(':')[1].split('/'); | ||
| this.#before_query = url.split('://')[1].split(':')[1].split('/').slice(1, -1).concat(url.split('://')[1].split(':')[1].split('/').reverse()[0].split('?')[0]); |
There was a problem hiding this comment.
코드만 딱 보았을때 어떤 부분을 어떻게 골라낼 의도인지 바로 보이지 않아서 아쉽네요! 이 부분에 관한 처리는 메소드로 따로 빼서 변수 네이밍을 활용하면 이해가 더 쉽지 않을까 조심스레 제안해봅니당
GiPyoo
requested changes
Jan 30, 2021
Comment on lines
+6
to
+17
| console.log(up.show()); | ||
| console.log(`URI: ${up.getURI()}`); | ||
| console.log(`Protocol: ${up.getProtocol()}`); //http | ||
| console.log(`Host: ${up.getHost()}`); //opentutorials.org | ||
| console.log(`Port: ${up.getPort()}`); //3000 | ||
| // 리턴이 void 라서 undefined 뜸 | ||
| // console.log(`Append Path : ${up.appendPath('econovation')}`); | ||
| console.log(`LastPath: ${up.removeLastPath()}`); //path | ||
| console.log(`Path: ${up.getPath()}`); //[main, path] path를 배열로 반환 | ||
| console.log(`Full Path: ${up.getFullPath()}`); | ||
| console.log(`Query String: ${up.getQueryString()}`); | ||
| console.log(`Query String Value: ${up.getQueryStringValueOf('page')}`); |
There was a problem hiding this comment.
Jest 라이브러리를 사용해보는 것은 어떠신가요?
테스팅 라이브러리 입니다.
Comment on lines
+46
to
+50
| let fullPath = ''; | ||
| for(let i of this.#full_path) { | ||
| fullPath += ('/'+i); | ||
| } | ||
| return fullPath; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
HELLO!
전에 필요없는 커밋들이 함께 들어가서 새브랜치로 재 pr 날립니다.
(체리픽, 리베이스, 다해봤는데, 뭔가 꼬인듯 합닏,....)
효율적이지 못한 부분이 있어서, 다른 분들 코드보고 참고하고 싶습니다.
퀘스트 내주신 @GiPyoo님과 완료하신 @vvvpiano 님께 리뷰 부탁드립니다 :)
