Skip to content
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

장준영 과제 작업완료 #3

Open
wants to merge 13 commits into
base: wns0901
Choose a base branch
from

Conversation

wns0901
Copy link

@wns0901 wns0901 commented Dec 15, 2022

No description provided.

@wns0901 wns0901 changed the title 장준영 백엔드 작업완료 장준영 과제 작업완료 Dec 15, 2022
@@ -0,0 +1,104 @@
# Logs

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.gitignore 파일은 일반적으로 github에 업로드하지 않습니다!
mini때는 업로드 안하는 방향으로 작업해주세요 :)

"bin": {
"login-lecture": "bin/www.js"
},
"dependencies": {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

설치된 모듈들이 굉장히 많네요...
사용하지 않는 모듈들은 삭제해주세요
나중에 미니, 메인 프로젝트를 진행하면 의존성 문제가 생길 수 도 있고 parkage.json파일을 관리할 때의 가독성도 떨어집니다!!

@@ -0,0 +1,16 @@
const mysql = require("mysql");
const dotenv = require("dotenv");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dotenv는 app.js에서 이미 호출해서 해당 파일에서 재호출 안해도되요 :)

if (err) {
console.log("생성 실패");
console.log(err);
reject({ success: false, msg: "일정 생성 실패" });

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위의 getToDoList랑 err처리 로직이 많이 다른데 형식은 되도록 통일시켜 주세요
그리고 console.log가 2줄이 있고 reject에도 일정 생성 실패가 있는데
일반적으로는 reject만 에러 검출을 합니다.
만약 console.log를 꼭 사용하고 싶으면 console.log("생성 실패", err); 한 줄로 처리할 수 있습니다 :)

const process = {
getToDoList: async () => {
const response = await ToDoData.getToDoList();
// console.log(response);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

작업 완료가 되면 console.log들은 지워주세요 :)

console.log(err);
reject({ success: false, msg: "일정 생성 실패" });
}
console.log("생성 성공");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolve로 success를 반환하고 있기 때문에
구현이 끝났으면 console.log들은 지원주세요 :)

static deleteData(data) {
return new Promise((resolve, reject) => {
const sql = "DELETE FROM todolist WHERE id= ?";
db.query(sql, [data.id], (err) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleteData로직을 보면 data객체에서 id값만 사용하고 있으니
매개변수로 data가 아닌 data.id를 넘겨주는 형식이 더 좋습니다 :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data를 넘기면 id말고 다른 값들도 같이 넘어오는데 그 값들은 사용되지 않고 버려지기 때문에 불필요한 리소스를 사용하게 됩니다!

try {
if (client.description.length > 16) {
return { success: false, msg: "글의 길이가 너무 깁니다." };
} else if (client.description === "") {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

컨벤션에서 if -> else if로 이어지는 형식을 잘 사용하지 않습니다
혹시 모르니까 제가 컨벤션 헷갈리 때 보는 링크 드릴게요!
https://github.com/tipjs/javascript-style-guide

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그리고 client.description === "" 란 코드는
!client.description 이 코드와 같은 의미인데 빈 값을 체크할 때는 이 방식이 더 좋습니다 :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

마지막으로 빈 값을 먼저 체크하고 길이제한 체크하는게 더 좋아요!

if (client.description.length > 16) {
return { success: false, msg: "글의 길이가 너무 깁니다." };
} else if (client.description === "") {
console.log("실행");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

console.log 발견!

.then((res) => {
console.log(res);
if (res.success) {
// alert("수정성공");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주석 발견!

const req = {
id: id,
};
// console.log(data);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주석 발견!

.then((res) => {
console.log(res);
if (res.success) {
// alert("삭제성공");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주석 발견!

}
})
.catch((err) => {
console.error(new Error("에러 발생"));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw new Error('에러 발생')
같은 형식의 메세지가 더 좋습니다

아니면 console.error(err);
하셔도 원하는 결과를 얻으실 수 있을 거에요

},

deleteList: async (req, res) => {
// console.log(req.body);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

console.log 발견!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

피드백 감사합니다. 말씀해 주신 내용 최대한 수정했습니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants