[yechoi] javascript-subway-map-precourse #3
Conversation
yechoi42
commented
May 27, 2021
•
yechoi42
commented
- MVC 패턴을 최대한 적용하려 했습니다.
- 이벤트 리스너에서 스코프 범위로 인해 나타나는 오류를 해결하기 위해, 이벤트 콜백 함수는 가급적 화살표 함수로 했습니다.
- View에서 화면을 렌더링 할 때는 removeChild를 활용했습니다.
- map, filter 등의 순회 함수를 적극적으로 사용하고자 했습니다.
- 각 element의 id나 class는 constant.js에 상수로 넣어 관리했습니다.
- 커밋의 단위를 기능을 기준으로 잘게 하려고 했습니다. 하지만 다른분들의 커밋메세지를 보니 커밋 단위를 더 잘게 쪼개야할 것 같네요.
- controller 내부에서 view를 호출하는 구조를 개선하여, 초기 화면 구성은 view의 constructor에서 하도록 수정했습니다.
- 이벤트가 중첩됐을 때 버그가 생겨서, controller 끼리 중첩되지 않도록 수정했습니다.
| @@ -0,0 +1,2 @@ | |||
| node_modules | |||
| .eslintrc.js | |||
There was a problem hiding this comment.
'EOL' 이라는 키워드를 검색해서 관련된 아티클을 한번 읽어보시면 좋을거 같아요!
| > 가장 먼저 버튼 각각에 독립된 핸들러를 할당하는 방법이 떠오를 겁니다. 하지만 이 방법보다 더 우아한 해결책이 있습니다. 메뉴 전체에 핸들러를 하나 추가해주고, 각 버튼의 data-action 속성에 호출할 메서드를 할당해 주는 방법 말이죠. | ||
|
|
||
|
|
||
| ### to do |
| "prettier": "^2.3.0" | ||
| }, | ||
| "scripts": { | ||
| "lint": "eslint ." |
|
|
||
| function activateTableListener() { | ||
| document.getElementById(Line.TABLE).addEventListener('click', (e) => { | ||
| const target = e.target; |
There was a problem hiding this comment.
처음에 코드를 작성하면서 매개 변수로 넘겨져오는 event 를 사용을 안했는데 생각해보니 event로 다 접근 가능 하더라구요..! document.~ 으로 접근안하고 event 활용한점 너무 좋은것같아요!
| const newStation = document.getElementById(S.INPUT).value; | ||
| if (newStation.length < 2) { | ||
| document.getElementById(S.INPUT).value = ''; | ||
| alert('지하철 역은 2글자 이상이어야 합니다.'); |
There was a problem hiding this comment.
| return true; | ||
| } else { | ||
| let stations = JSON.parse(oldStations); | ||
| if (stations.includes(newStation) === false) { |
There was a problem hiding this comment.
includes 반환값이 boolean인 만큼 다시 false로 명시해주기보다는 명시하지 않는 편이 가독성 측면에서 좋은것같아서 리뷰했습니다.
| function checkLineIncludedStation(lines, station) { | ||
| let ret = lines.every((line) => { | ||
| if ( | ||
| line.stations.filter((item) => item !== station).length != | ||
| line.stations.length | ||
| ) { | ||
| return false; | ||
| } else { | ||
| return true; | ||
| } | ||
| }); | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
| function checkLineIncludedStation(lines, station) { | |
| let ret = lines.every((line) => { | |
| if ( | |
| line.stations.filter((item) => item !== station).length != | |
| line.stations.length | |
| ) { | |
| return false; | |
| } else { | |
| return true; | |
| } | |
| }); | |
| return ret; | |
| } | |
| function checkLineIncludedStation(lines, station) { | |
| return lines.every((line) => line.stations.filter((item) => item !== station).length === line.stations.length) | |
| } |
다음 코드가 가독성 측면에서 조금 더 좋은 것 같아서 리뷰했습니다.
| export function deleteStation(station) { | ||
| let oldStations = JSON.parse(localStorage.getItem('stations')); | ||
| let lines = JSON.parse(localStorage.getItem('lines')); | ||
|
|
||
| if (checkLineIncludedStation(lines, station) === false) { | ||
| return false; | ||
| } else { | ||
| const stations = oldStations.filter((element) => element != station); | ||
| localStorage.setItem('stations', JSON.stringify(stations)); | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
let oldStations = JSON.parse(localStorage.getItem('stations'));
let lines = JSON.parse(localStorage.getItem('lines'));
다음과 같이 데이터 정제 부분을 model 로직으로 뺀다면 조금 더 로직을 단순화 시킬 수 있을것같아서 리뷰했습니다. 사실 oldStations와 lines는 함수 내에서 로직이 복잡하지않고 한번밖에 쓰이지 않기때문에 변수에 할당하기보다는 바로 명시해주는게 좋다고 생각합니다.
또한, 함수 최상단에 return이 없기때문에 로직에는 문제가 없을지라도 가독성 측면에서 많이 떨어진다고 생각합니다.
checkLineIncludedStation 같은 경우 함수 이름내에 충분히 기능이 내포돼있으므로 checkLineIncludedStation(lines, station) === false 보다는 !checkLineIncludedStation(lines, station) 가 가독성 측면에서 조금 더 좋다고 생각해서 리뷰했습니다.
| export function deleteStation(station) { | |
| let oldStations = JSON.parse(localStorage.getItem('stations')); | |
| let lines = JSON.parse(localStorage.getItem('lines')); | |
| if (checkLineIncludedStation(lines, station) === false) { | |
| return false; | |
| } else { | |
| const stations = oldStations.filter((element) => element != station); | |
| localStorage.setItem('stations', JSON.stringify(stations)); | |
| return true; | |
| } | |
| } | |
| export function deleteStation(station) { | |
| if (!checkLineIncludedStation(lines, station)) { | |
| return false; | |
| } | |
| localStorage.setItem('stations', JSON.stringify(oldStations.filter((v) => v != station))); | |
| return true; | |
| } |
| const table = document.getElementById(Station.TABLE); | ||
| table.insertAdjacentHTML( | ||
| 'beforeend', | ||
| `<tr> |
There was a problem hiding this comment.
하나의 표에 다수의 를 연속적으로 사용할 수 있으며, 이를 통해 커다란 표의 행을 구획으로 나눌 수 있습니다. 필요한 경우 각 구획에 서로 다른 서식을 적용할 수도 있습니다.
오.. tbody가 table안에 여러개 존재할 수 있군요! 새로 알았습니다.
현재 tbody, thead를 명시하지 않고 생성해서 모두 tbody로 생성이돼서,
노선 이름 | 상행 종점역 | 하행 종점역 | 설정이 tbody로 되었는데, thead로 묶어주면 조금 더 접근성 측면에서 좋을 것같아요!
또, 표의 단위가 작은 만큼 tbody 하나 안에 원소들을 추가하는것도 좋을것 같아서 리뷰했습니다.
| if (action === 'showSectionManager') { | ||
| renderSectionManager(target.dataset.id); | ||
| document | ||
| .getElementById(Section.REGISTER) |
There was a problem hiding this comment.
getElementById의 인자를 define해놓으니 가독성이 훨씬 좋네요
| return JSON.parse(localStorage.getItem('lines')); | ||
| } | ||
|
|
||
| export function addLine(newLineName, start, end) { |
There was a problem hiding this comment.
addLine 함수의 성공 실패 여부를 boolean함수를 만들어 먼저 필터링해주면 어떨까 하는 생각이 듭니다.
| } | ||
|
|
||
| export function constructLine() { | ||
| const stations = JSON.parse(localStorage.getItem('stations')); |
There was a problem hiding this comment.
로컬스토리지 접근 함수를 코드 전반에 걸쳐 여러차례 사용하기 때문에 미리 유틸에 넣어놓는 방법도 좋을 것 같습니다.
export const storeLocalStorage = (key, object) => {
localStorage.setItem(key, JSON.stringify(object));
};
export const loadLocalStorage = (key) => {
return JSON.parse(localStorage.getItem(key));
};| // show 함수들이 다 내부적으로 비슷한데 효율적으로 만들 방법은 없었을까? | ||
| // getParts를 매번 해오지 않을 방법은? object 다른 파일에서 import 해서 쓰는 건 잘 작동하지 않았다. | ||
| // 초기에 id가 설정이 안돼있었기 때문일까? |
There was a problem hiding this comment.
toggle 관련해서 비슷한 고민을 했는데 좋은 방법을 찾으면 공유하기로...!
#1 (comment)
| `<tr><td style="border:1px solid;">${station}</td> | ||
| <td style="border:1px solid;"><button data-id="${station}" data-action="deleteStation">삭제</button></td></tr>`, |
| stationAddButton.addEventListener('click', () => clickAddStation()); | ||
| document.getElementById(S.TABLE).addEventListener('click', (e) => { | ||
| const target = e.target; | ||
| const action = target.dataset.action; |
There was a problem hiding this comment.
dataset에 action을 정의하여 관련 함수를 호출할 수 있게끔 하는 방법이 좋은 것 같습니다.
