6주차 news api 실습 리뷰를 바탕으로 리팩토링 #23

chl9430 · 2022-05-31T01:18:44Z

This reverts commit cb092d4.

📚 개요

📚 작업사항 (기능추가/수정/삭제 등)

I made the async/await function in my code and make it easy to understand.

📚 리팩토링 반영 사항

Add async/await function to request API data.
I made more functions that have only a single role.
Normalize a user's search keyword.

This reverts commit cb092d4.

Seok93

리팩토링 고생하셨습니다. 다른 부분도 한 번 참고해주세요~

Seok93 · 2022-06-06T02:28:35Z

week6/jung-hoo/main.js

+function convertString(string) {
+  return string.toUpperCase();
+  // 대소문자를 구분하지 않기 위해서 잠시 대문자로 바꾼다.
+}


굳이 함수로 나누지 않아도 될 것 같네요 ㅎㅎ

코드를 참고하면 convertString(newsData.articles[i].title).includes(searchKeyword) 이 부분에서 사용하고 있지요?
newsData.articles[i].title.toUpperCase().includes('sarchKeyword) 이렇게 바로 붙여서 호출 할 수 있겠네요.

만약에 toUpperCase 함수 이외의 추가적인 작업이 있었다면 함수로 분리하는 것을 고려해볼만하지만, 하나라면 나누지 않는게 좋다고 생각합니다.

또는 includes까지 분리된 함수로 이동하여 함수의 역할을 보다 강화하면 좋을 것 같아요.
사용처에서 모두 includes 여부를 판별하고 있는데 이 역할이 반복되고 있다보니 공통화 가능해 보입니다.
다만 역할을 고려하여 인자가 추가로 필요할 것이고 네이밍도 적절하게 변경되어야 할 것 같습니다!

Seok93 · 2022-06-06T02:30:47Z

week6/jung-hoo/main.js

+    `https://newsapi.org/v2/everything?q=${searchKeyword}&sortBy=popularity&apiKey=2c157850e4c64fd99aaf77d60228d630`
+  );
+  const newsData = await res.json();
+  console.log(newsData);


#12 (comment) 같은 내용으로 리뷰했어요! 참고하면 좋을 것 같습니다.

Seok93 · 2022-06-06T02:32:21Z

week6/jung-hoo/main.js

+  if (newsData.articles.length === 0) {
+    // 아예 기사를 못가져왔을 경우
+    alert("검색결과가 없습니다!"); // 경고창을 띄운다.
+  }


articles 내용이 없어서, alert함수로 검색 결과가 없다고 출력했다면, 이후에 return; 을 통해 함수의 실행을 중단할 필요가 있겠네요. 그렇지 않으면 결국 하단의 코드를 다 실행한 후에 끝이 나겠죠?

Seok93 · 2022-06-06T02:33:23Z

week6/jung-hoo/main.js

+    if (i === newsData.articles.length - 1) {
+      // 마지막 탐색을 마친 후
+      if (resBoxEl.innerHTML === "") {
+        // 가져온 기사들 중 조건에 부합하는 기사가 없다면...
+        alert("검색결과가 없습니다!"); // 경고창을 띄운다.
+      }
+    }


42~45줄에서 articles이 없다고 인식했을 때 return 했다면 필요없는 내용이 되겠네요.

Seok93 · 2022-06-06T02:35:51Z

week6/jung-hoo/main.js

+  // 검색결과는 대소문자를 상관하지 않기때문에 입력값을 우선 대문자로 바꾼다.
+  // 그 후, 문자열 시작과 끝에 공백을 제거한다.


개별 개발을 진행할 때 일의 순서와 역할을 주석으로 작성하면서 개발하는 것은 상관없습니다. 하지만 이런 주석들을 모두 협업 repo에 넣으면, 파일의 용량도 커지고 불필요한 주석이 많아서 가독성이 떨어질 수도 있으며, 정작 중요한 주석은 못읽을 수도 있습니다.

팀원과 작업을 할 때에는 정말 필요한 주석만 작성해서 넣어주시면 될 것 같아요!

참고로 번들러 사용하시면 나중에 production build에서는 주석 제외할 수 있습니다!
또한 중요한 주석에 대해서는 중요 주석임을 표시해주셔도 좋을 것 같아요. 예를 들어 다음과 같은 주석 prefix들은 vscode highlighter 익스텐션도 제공될 정도로 많이 쓰입니다.
// TODO -> 추가 작업이 필요한 부분일 때
// FIXME -> 버그 픽스가 필요한 부분일 때

Seok93 · 2022-06-06T02:41:20Z

week6/jung-hoo/main.js

+    alert("검색결과가 없습니다!"); // 경고창을 띄운다.
+  }
+
+  for (let i = 0; i < newsData.articles.length; ++i) {


for문도 좋지만, newsData.articles가 배열 데이터이니, 배열 내장함수들을 이용해보는건 어떨까요?

추가로 articles 를 사용할 때 체이닝 길이가 깊어져서 사용하기 힘드네요. ES6 구조분해 할당을 이용해서 articles를 빼내어서 사용해보는 것도 좋지 않을까요?

const { articles } = newsData;
for(let i = 0; i < articles.length; ++i)

Seok93 · 2022-06-06T02:48:38Z

week6/jung-hoo/main.js

+    if (filterEl.value === "제목") {
+      // 검색 조건이 제목이라면...
+      if (
+        convertString(newsData.articles[i].title).includes(searchKeyword)
+        // 키워드가 포함되어 있는지 알아본다.
+        // 그래서 입력한 값이 기사의 제목에 포함되어 있다면...
+      ) {
+        // true 값을 반환한 i번째 기사의 제목과 내용, url를 표시한다.
+        revealArticle(newsData.articles[i], resBoxEl);
+      }
+    } else if (filterEl.value === "내용") {
+      // 검색 조건이 내용이라면...
+      if (
+        // 입력한 값이 기사의 내용에 포함되어 있다면...
+        convertString(newsData.articles[i].content).includes(searchKeyword)
+        // 키워드가 포함되어 있는지 알아본다.
+        // 그래서 입력한 값이 기사의 내용에 포함되어 있다면...
+      ) {
+        // true 값을 반환한 i번째 기사의 제목과 내용, url를 표시한다.
+        revealArticle(newsData.articles[i], resBoxEl);
+      }
+    }


if문을 내부로 넣어 depth를 깊게 할 필요가 없는 것 같아요.
&& 를 이용하면 단축연산이 일어나기 때문에 조건 검사가 조기에 끝날수도 있으니 위와 같은 방식이라 할 수 있겠네요!

if ( filterEl.value === "제목" && convertString(newsData.articles[i].title).includes(searchKeyword) ) { revealArticle(newsData.articles[i], resBoxEl); } else if ( filterEl.value === "내용" && convertString(newsData.articles[i].content).includes(searchKeyword) ) { revealArticle(newsData.articles[i], resBoxEl); }

includes 결과값이 false일때 무언가 처리가 필요해진다면, 현재의 구조로 다시 되돌아 와야 할 것 같아요.
따라서 필요에 따라 현재 구조를 유지할지 && 연산자를 사용할지 결정되면 좋을 것 같습니다.
&& 연산자의 경우 else 처리가 복잡해질 때는 엣지케이스가 생기기 쉬운 것 같기도 해요!
만약 유지하되 depth를 줄이고 싶다면 두번째 if문부터는 별도 함수로 분리해볼수 있을 것 같습니다. processTitleSearch, processContentSearch 등..?
추가로 조건으로 참조되는 값들은 체이닝까지 포함되어 있으면 더욱 복잡하게 느껴져서 변수화하시면 보다 가독성에 도움이 됩니다.
예) const isIncludingKeyword = convertString(newsData.articles[i].title).includes(searchKeyword);

danbileee

수고 많으셨습니다!

danbileee · 2022-06-06T07:20:16Z

week6/jung-hoo/main.js

+  // 검색결과는 대소문자를 상관하지 않기때문에 입력값을 우선 대문자로 바꾼다.
+  // 그 후, 문자열 시작과 끝에 공백을 제거한다.


참고로 번들러 사용하시면 나중에 production build에서는 주석 제외할 수 있습니다!
또한 중요한 주석에 대해서는 중요 주석임을 표시해주셔도 좋을 것 같아요. 예를 들어 다음과 같은 주석 prefix들은 vscode highlighter 익스텐션도 제공될 정도로 많이 쓰입니다.
// TODO -> 추가 작업이 필요한 부분일 때
// FIXME -> 버그 픽스가 필요한 부분일 때

danbileee · 2022-06-06T07:20:46Z

week6/jung-hoo/main.js

+  targetEl.innerHTML += `<h3>${data.title}</h3>
+                    <div>${data.content}<br/>
+                    <a href="${data.url}">See more</a></div>`;


사소하지만 innerHTML 내부에서도 indent는 지켜주시면 더 좋을 것 같습니다~!

danbileee · 2022-06-06T07:23:19Z

week6/jung-hoo/main.js

+function convertString(string) {
+  return string.toUpperCase();
+  // 대소문자를 구분하지 않기 위해서 잠시 대문자로 바꾼다.
+}


또는 includes까지 분리된 함수로 이동하여 함수의 역할을 보다 강화하면 좋을 것 같아요.
사용처에서 모두 includes 여부를 판별하고 있는데 이 역할이 반복되고 있다보니 공통화 가능해 보입니다.
다만 역할을 고려하여 인자가 추가로 필요할 것이고 네이밍도 적절하게 변경되어야 할 것 같습니다!

danbileee · 2022-06-06T07:30:15Z

week6/jung-hoo/main.js

+    if (filterEl.value === "제목") {
+      // 검색 조건이 제목이라면...
+      if (
+        convertString(newsData.articles[i].title).includes(searchKeyword)
+        // 키워드가 포함되어 있는지 알아본다.
+        // 그래서 입력한 값이 기사의 제목에 포함되어 있다면...
+      ) {
+        // true 값을 반환한 i번째 기사의 제목과 내용, url를 표시한다.
+        revealArticle(newsData.articles[i], resBoxEl);
+      }
+    } else if (filterEl.value === "내용") {
+      // 검색 조건이 내용이라면...
+      if (
+        // 입력한 값이 기사의 내용에 포함되어 있다면...
+        convertString(newsData.articles[i].content).includes(searchKeyword)
+        // 키워드가 포함되어 있는지 알아본다.
+        // 그래서 입력한 값이 기사의 내용에 포함되어 있다면...
+      ) {
+        // true 값을 반환한 i번째 기사의 제목과 내용, url를 표시한다.
+        revealArticle(newsData.articles[i], resBoxEl);
+      }
+    }


includes 결과값이 false일때 무언가 처리가 필요해진다면, 현재의 구조로 다시 되돌아 와야 할 것 같아요.
따라서 필요에 따라 현재 구조를 유지할지 && 연산자를 사용할지 결정되면 좋을 것 같습니다.
&& 연산자의 경우 else 처리가 복잡해질 때는 엣지케이스가 생기기 쉬운 것 같기도 해요!
만약 유지하되 depth를 줄이고 싶다면 두번째 if문부터는 별도 함수로 분리해볼수 있을 것 같습니다. processTitleSearch, processContentSearch 등..?
추가로 조건으로 참조되는 값들은 체이닝까지 포함되어 있으면 더욱 복잡하게 느껴져서 변수화하시면 보다 가독성에 도움이 됩니다.
예) const isIncludingKeyword = convertString(newsData.articles[i].title).includes(searchKeyword);

Revert "Revert "feat: I created recent-news-search-app with vanila JS""

052ee85

This reverts commit cb092d4.

chl9430 requested a review from danbileee May 31, 2022 01:19

chl9430 self-assigned this May 31, 2022

chl9430 added Status: Request 리뷰 요청 Type: Refactor 코드 리팩토링 labels May 31, 2022

Seok93 reviewed Jun 6, 2022

View reviewed changes

danbileee reviewed Jun 6, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6주차 news api 실습 리뷰를 바탕으로 리팩토링 #23

6주차 news api 실습 리뷰를 바탕으로 리팩토링 #23

chl9430 commented May 31, 2022 •

edited

Loading

Seok93 left a comment

Seok93 Jun 6, 2022 •

edited

Loading

danbileee Jun 6, 2022

Seok93 Jun 6, 2022

Seok93 Jun 6, 2022

Seok93 Jun 6, 2022

Seok93 Jun 6, 2022

danbileee Jun 6, 2022

Seok93 Jun 6, 2022

Seok93 Jun 6, 2022 •

edited

Loading

Seok93 Jun 6, 2022

danbileee Jun 6, 2022

danbileee left a comment

danbileee Jun 6, 2022

danbileee Jun 6, 2022

danbileee Jun 6, 2022

danbileee Jun 6, 2022

		// 검색결과는 대소문자를 상관하지 않기때문에 입력값을 우선 대문자로 바꾼다.
		// 그 후, 문자열 시작과 끝에 공백을 제거한다.

6주차 news api 실습 리뷰를 바탕으로 리팩토링 #23

Are you sure you want to change the base?

6주차 news api 실습 리뷰를 바탕으로 리팩토링 #23

Conversation

chl9430 commented May 31, 2022 • edited Loading

📚 개요

📚 작업사항 (기능추가/수정/삭제 등)

📚 리팩토링 반영 사항

Seok93 left a comment

Choose a reason for hiding this comment

Seok93 Jun 6, 2022 • edited 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

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

Seok93 Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbileee 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

chl9430 commented May 31, 2022 •

edited

Loading

Seok93 Jun 6, 2022 •

edited

Loading

Seok93 Jun 6, 2022 •

edited

Loading