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

[Feat/#250] 탭투탭에서 블록이 겹쳐서 쌓이는 문제 해결 #251

Merged
merged 5 commits into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,52 +1,78 @@
import { useSelectContext } from 'pages/selectSchedule/contexts/useSelectContext';

const useSlotSeletion = () => {
const {startSlot, setStartSlot, selectedSlots, setSelectedSlots} = useSelectContext();
const { startSlot, setStartSlot, selectedSlots, setSelectedSlots } = useSelectContext();

const handleSelectSlot = (targetSlot: string) => {
setStartSlot(targetSlot);
}
const handleSelectSlot = (targetSlot: string) => {
setStartSlot(targetSlot);
};

const handleCompleteSlot = (targetSlot: string) => {
const dateOfStartSlot = startSlot?.substring(0, startSlot.lastIndexOf('/'));
const dateOfTargetSlot = targetSlot.substring(0, targetSlot.lastIndexOf('/'))
if (startSlot && dateOfStartSlot === dateOfTargetSlot){
const newSelectedSlot = {
date:dateOfStartSlot,
startSlot:startSlot?.substring(startSlot.lastIndexOf('/')+1),
endSlot:targetSlot.substring(targetSlot.lastIndexOf('/')+1),
priority:0,
}

const keys = Object.keys(selectedSlots).map(Number)
const newKey = keys.length ? Math.max(...keys) + 1 : 0;
const newSelectedSlots = {...selectedSlots};
newSelectedSlots[newKey] = newSelectedSlot;
setSelectedSlots(newSelectedSlots)
}
setStartSlot(undefined);
}
const handleCompleteSlot = (endSlot: string) => {
const dateOfStartSlot = startSlot && startSlot.substring(0, startSlot.lastIndexOf('/'));
const dateOfEndSlot = endSlot.substring(0, endSlot.lastIndexOf('/'));
Comment on lines +11 to +12
Copy link
Member

Choose a reason for hiding this comment

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

p4) 이 두 부분이 slot에 해당하는 date 즉 날짜를 가져오는 부분으로 이해했는데 이 두가지 변수가 다른 경우의 수가 있나요? 현재 저희 서비스에서는 날짜가 넘어가는 로직을 다루고 있진 않은 것 같아서요!

Copy link
Member Author

Choose a reason for hiding this comment

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

handleCompleteSlot()에서는 시작시간(startSlot)이 선택된 상태에서 종료시간(endSlot)을 선택했을 때 그게 유효한 선택인지를 확인해야합니다.
그래서 startSlot과 endSlot의 날짜가 일치한지를 비교해야합니다! 시작 시간은 월요일에 찍어두고 종료시간을 수요일에 찍으면 블럭이 만들어질 수 없으니까, 그렇지 않은 경우에만 블록을 생성하는 겁니다.

Copy link
Member

Choose a reason for hiding this comment

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

아하! 클릭한 두 날짜가 같아야만 블록을 생성하는 로직이군요! 이해했습니다. 감사합니다.

if (startSlot && dateOfStartSlot === dateOfEndSlot) {
const newSelectedSlot = {
date: dateOfStartSlot,
startSlot: startSlot && startSlot.substring(startSlot.lastIndexOf('/') + 1),
endSlot: endSlot.substring(endSlot.lastIndexOf('/') + 1),
priority: 0,
};

const handleDeleteSlot = (selectedEntryId: number) => {
const newSelectedSlots = {...selectedSlots};
delete newSelectedSlots[selectedEntryId];
setSelectedSlots(newSelectedSlots);
const keys = Object.keys(selectedSlots).map(Number);
const newKey = keys.length ? Math.max(...keys) + 1 : 0;
Copy link
Member

Choose a reason for hiding this comment

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

p4) key가 [0,1,2] 를 반환한다면 Math.max(...keys)+1 을 사용해서 연산을 한 번 더 하는 것보다,
keys.length을 활용해 아래처럼 활용하는 것은 어려울까요??
key값이 0부터 시작하는 것이 아니라면 반려해주세요!

const newKey = keys.length ? newKey + 1 : 0;

Copy link
Member Author

Choose a reason for hiding this comment

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

selectedSlots내의 객체들은 계속해서 생성됐다 삭제되기때문에 length가 마지막 key를 의미하지는 않습니다! 그래서 최대값에 +1을 하는 방식을 사용했습니닷!

Copy link
Member

Choose a reason for hiding this comment

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

아하 이해했습니다. 감사합니다!


setSelectedSlots((prev) => {
const newSelectedSlots = { ...prev };
newSelectedSlots[newKey] = newSelectedSlot;
return newSelectedSlots;
});
removeOverlappedSlots(endSlot, dateOfStartSlot);
}
setStartSlot(undefined);
Copy link
Member

Choose a reason for hiding this comment

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

p2) undefined값은 개발자가 의도적으로 할당하는 값이 아닌 자바스크립트 엔진에서 초기화된 값으로 알고 있습니다. 즉 값이 없음을 나타내는 것이 아닌 초기화되지 않음 을 나타내는것으로 알고있습니다.
값이 없음을 나타내기 위해선 null 을 사용하는것은 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

오 생각지 못했던 부분이에요!!!
관련해서 블로그 글이나 그런건 많은데 공식적인 출처는 잘 안보여서 아쉽네요... 더 정확히 알게된다면 공유드리겠습니다
이 글에서 서적에 있는 내용을 인용한 걸 보았을 땐 '나중에 값을 저장하기 위한 변수를 정의한다면, 해당 변수를 다른 값이 아닌 null로 초기화하는 것이 좋다. 이렇게 하면 나중에 변수에 값이 할당됐는지 여부를 명시적으로 확인할 수 있다' 라고 언급하고있네요!
재훈님 말대로 null을 사용하는 것이 맞는 것 같습니다.
(그러나 수많은 코드에서 모두 undefined로 초기화해버린 나...... )

Copy link
Member

@ljh0608 ljh0608 Jul 7, 2024

Choose a reason for hiding this comment

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

좋습니다! 말씀하신대로 "null과 undefined의 차이" 에 관한 블로그 글은 많지만 저 역시 궁금하여 정확한 근거를 찾아보았는데요. mozilla에서도 null은 의도적으로 값이 비어있음을 의미한다면 undefined는 값이 할당되지 않은 변수 (초기화하지 않은 변수)의 값을 나타낸다고 합니다.
이부분 수정해주시면 approve하겠습니다!

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/null
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/undefined

};

const handleDeleteSlot = (selectedEntryId: number) => {
setSelectedSlots((prev) => {
const newSelectedSlots = { ...prev };
delete newSelectedSlots[selectedEntryId];
return newSelectedSlots;
});
};

const onClickSlot = (targetSlot:string, selectedEntryId?:number)=>{
if (selectedEntryId !== undefined){
if (startSlot === undefined){
handleDeleteSlot(selectedEntryId);
}
setStartSlot(undefined)
} else if (startSlot !== undefined){
handleCompleteSlot(targetSlot)
} else {
handleSelectSlot(targetSlot)
const removeOverlappedSlots = (endSlot: string, dateOfStartSlot: string) => {
const selectedSlotsPerDate = Object.fromEntries(
Object.entries(selectedSlots).filter(([, slot]) => slot.date === dateOfStartSlot),
);
Object.entries(selectedSlotsPerDate).forEach(
Copy link
Member

Choose a reason for hiding this comment

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

p2) 이부분 fromEntries 연산과 entries연산이 불필요하게 들어갔다고 생각합니다. 아래처럼 활용했을 때의 문제점이 있을까요? 혹시 다른 의도가 있으셨다면 말씀해주세요!

    const selectedSlotsPerDate = 
      Object.entries(selectedSlots).filter(([, slot]) => slot.date === dateOfStartSlot);
     
     selectedSlotsPerDate.forEach(
     ...

Copy link
Member Author

Choose a reason for hiding this comment

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

앗 맞습니다!! 매의 눈 미쳤다!!

([id, { startSlot: selectedStartSlot, endSlot: selectedEndSlot }]) => {
const startSlotTime = startSlot && startSlot.split('/').pop();
const endSlotTime = endSlot.split('/').pop();
if (
startSlotTime &&
endSlotTime &&
selectedStartSlot > startSlotTime &&
selectedEndSlot < endSlotTime
) {
handleDeleteSlot(Number(id));
}
Copy link
Member

@ljh0608 ljh0608 Jul 6, 2024

Choose a reason for hiding this comment

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

p2) 이부분은 설계 부분에서 의문점이 하나 들었는데요! 이러면 기존에 있던 A블럭보다 일찍 시작하고, 늦게끝나는 B블럭(확실히 포함하는 블럭)을 생성하면 지워지는 로직은 잘 구현된 것 같습니다. 하지만 7시-10시의 C블럭이 있는 상태에서 6시-9시 D블럭을 생성하게 되는 예외처리는 어떻게 되는지 궁금합니다! 현재 로직을 이렇게 변경하면 시간이 겹치는케이스까지 핸들링할 수 있지 않을까요?

  startSlotTime &&
          endSlotTime &&
          selectedStartSlot > startSlotTime &&
          selectedStartSlot < endSlotTime

추가로 selectedStartSlot변수와 startSlotTime을 이해하는데 조금 혼동이 있었습니다.
startSlotTime의 변수명을 currentStartSlotTime으로 변경해보는건 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

해당 예외케이스는 이미 처리되고 있습니다! 말씀해주신 상황처럼 7시-10시가 선택되어있는 상태에서, 6시를 시작시간으로 누르고 종료시간으로 9시를 누르려고 한다면, 9시는 이미 생성된 블럭 내에 있기 때문에 이는 유효하지 않은 선택으로 파악되어 그냥 시작시간을 선택한 상태를 무효화합니다. (아래 영상 참고!)

2024-07-06.10.48.00.mov

Copy link
Member Author

Choose a reason for hiding this comment

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

변수명 가독성 저도 공감해서 반영했습다!

Copy link
Member

Choose a reason for hiding this comment

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

변수명 반영 감사합니다!
예외처리도 확인하였는데 그렇다면 이 경우에 6시부터 8시까지로 시간을 변경하고 싶으면 취소 하고 다시 등록하면 되겠군요!
저는 6시부터 8시의 블록을 새로 만드는 것으로 생각했는데 유저가 시간을 입력하고 지우는데에 큰 리소스가 드는 것이 아니라서 문제될 것이 없을 것 같네요! 고생하셨습니다~

},
);
};

const onClickSlot = (targetSlot: string, selectedEntryId?: number) => {
if (selectedEntryId !== undefined) {
if (startSlot === undefined) {
handleDeleteSlot(selectedEntryId);
}
setStartSlot(undefined);
} else if (startSlot !== undefined) {
Copy link
Member

@ljh0608 ljh0608 Jul 6, 2024

Choose a reason for hiding this comment

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

p3) 위에 리뷰와 마찬가지로 startSlot이나 Id값을 null로 핸들링 할 수 있을 것 같습니다! id와 같은경우에는 undefined가 필요할 수도 있을 것 같은데 만약 제가 로직상 놓친 부분이 있다면 한 번더 설명 덧붙여주시면 감사하겠습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

흐아 근데 startSlot이 사용된 모든 로직뿐만 아니라 제가 작성한 모든 코드에서 사실 null 대신 undefined를 쓰고있는데 이걸 전부 고쳐야하는걸까요? undefined를 사용하는 것에 너무 익숙해져있었는데 이게 모든 코드를 수정해야할 정도로 잘못된 사용인지에 대한 판단이 잘 안서네요...

Copy link
Member

@ljh0608 ljh0608 Jul 7, 2024

Choose a reason for hiding this comment

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

제 의견을 드리자면 이부분이 당장 급하게 수정해야할 급한 사안은 아니라고 생각이듭니다. 릴리즈까지 다른 해야할 일이 많다면 우선순위를 뒤에 둘 수 있을 것 같아요. 다만 지금 수정하지 않으면 추후 로직에서 모든 관련된 타입을 undefined로 선언해서 추후 유지보수 비용이 커질 수는 있다고 생각했습니다.

  const [startSlot, setStartSlot] = useState<string | undefined>(undefined);

제가 ECMA script 공식문서 TC39를 참고해보니 ECMA script 역시 이 두 값을 명확하게 구분하고 있습니다.
요약하자면 null = 의도적으로 객체 값이 없음을 나타내는 원시값 undefined 변수에 값이 할당되지 않았을 때 사용하는 원시값 입니다.

저희가 사용하는 로직에선 초기화 하기 이전으로 돌리는 것이 아닌 값이 비어있음을 나타내는 것이니 null의 사용이 적절하다고 생각하고 변수에 값이 메모리에 할당되기 이전에 자바스크립트 엔진은 undefined를 초기값으로 할당하는데 개발자가 직접 undefined를 할당한다면 이것이 선언 되었는데 값이 할당되기 전인지 개발자가 의도적으로 비워둔 것인지에 대한 분별력을 잃는다고 생각합니다. 아래 사진은 공식문서를 캡쳐한 사진입니다!

image
image

공식문서 링크

아래와 같은 글은 새로운 관점을 제시하고 있어서 참고용으로 하나 링크 남겨두도록 하겠습니다.
https://blog.shiren.dev/2021-10-05/

해당 아티클, 공식문서를 통해 제 생각이 담긴 null과 undefined에 대한 고찰이라는 글을 작성했는데 한번 읽어봐주시면 좋을 것 같습니다!
https://anstrengung-jh.tistory.com/103

Copy link
Member Author

Choose a reason for hiding this comment

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

맞습니다..! 좋은 자료들 공유 넘 감사드립니다 🚀🚀
그러면 우선 시간표내에 undefined로 초기화했던 것을 null로 바꾸도록 하겠습니다.

handleCompleteSlot(targetSlot);
} else {
handleSelectSlot(targetSlot);
}
};

return {startSlot, onClickSlot}
}
return { startSlot, onClickSlot };
};

export default useSlotSeletion
export default useSlotSeletion;
3 changes: 1 addition & 2 deletions src/pages/selectSchedule/utils/changeApiReq.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import { ScheduleStates } from 'pages/legacy/selectSchedule/types/Schedule';
import {
HostAvailableSchduleRequestType,
UserAvailableScheduleRequestType,
} from 'src/types/createAvailableSchduleType';

import { ScheduleStates } from '../types/Schedule';

export const transformHostScheduleType = (
scheduleList: ScheduleStates[],
): (HostAvailableSchduleRequestType | null)[] => {
Expand Down
Loading