-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/#22 주식 차트 데이터 조회 기능 추가 #163
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다! 👍
} | ||
|
||
@Injectable() | ||
export class StockDataYearlyService extends StockDataService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개별적인 클래스를 만드는 것보다 엔티티 타입을 바로 윗단에서 전달하는건 어떤가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그것도 좋을 것 같습니다 ! 사실 엔티티를 그냥 전달할 지 아니면 서비스 클래스로 구현을 할 지 고민을 했었습니다. 고민을 하면서 두 가지 이유로 이와 같이 구현을 하게 되었습니다.
- 주어진 필드는 동일하지만 다른 엔티티라고 생각해서 다른 서비스에서 로직이 진행이 되어야한다고 생각했습니다.
- 지금은 조회 기능뿐이지만 각 엔티티별로 세부적인 기능이 필요할 수 있으니 확장성 면에서 이게 좋지 않나 생각했습니다.
민수의 생각은 어떠신가요?
lastStartTime?: string, | ||
): Promise<StockDataResponse> { | ||
return await this.dataSource.manager.transaction(async (manager) => { | ||
if (!(await manager.exists(Stock, { where: { id: stock_id } }))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요부분은 함수로 빼는건 어떤가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그것도 좋을 것 같습니다 반영하겠습니다 !
.createQueryBuilder(entity, 'entity') | ||
.where('entity.stock_id = :stockId', { stockId: stock_id }) | ||
.orderBy('entity.startTime', 'DESC') | ||
.take(this.PAGE_SIZE + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
페이지 크기보다 1개 더 받는 이유가 다음 데이터가 있는지 확인하려는 의도인가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞습니다 !
import { Stock } from './stock.entity'; | ||
|
||
@Entity('stock_minutely') | ||
export class StockMinutely { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typeORM에서 상속을 적용해서 같은 칼럼이 겹치는 여러개의 엔티티의 코드를 줄일 수 있습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 이거 만들었는데 빨리 올릴걸... 두번 일하게 만들어 드렸네요 ㅠㅠ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
괜찮습니다 ! 그리고 엔티티 상속은 몰랐었네요 !
import { ApiOperation, ApiParam, ApiQuery, ApiResponse } from '@nestjs/swagger'; | ||
import { StockDataResponse } from '../dto/stockData.response'; | ||
|
||
export function ApiGetStockData(summary: string, type: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 방법으로 swagger 데코레이터를 따로 분리할 수 있네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
swagger 데코레이터가 길어 져서 ESLint에 걸려서 분리하는 방법을 찾다보니 이런 방법이 있구나 싶었습니다 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드가 깔끔하고, 커스텀 데코레이터를 사용해서 처리하셨습니다!
@OneToOne(() => Stock) | ||
@JoinColumn({ name: 'stock_id' }) | ||
stock: Stock; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이게 1:다 관계가 될 수 있더라고요 ㅠ..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이게 1:N 관계가 될 수가 있군요! 어떤 관계에서 그렇게 될까요?
close #22
✅ 작업 내용
✍ 궁금한 점
😎 체크 사항