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

[enhance] CheckEvent controller with new query endpoints #32

Merged
merged 6 commits into from
Jan 7, 2025

Conversation

Soecka
Copy link
Contributor

@Soecka Soecka commented Jan 3, 2025

PR-32 PR-32 PR-32 Powered by Pull Request Badge

Add CheckEvent controller with new query endpoints for user and activity check events

@Soecka Soecka added the enhancement Some improvements label Jan 3, 2025
@Soecka Soecka requested a review from TechQuery January 3, 2025 13:47
@TechQuery TechQuery added feature New feature or request and removed enhancement Some improvements labels Jan 4, 2025
@Soecka Soecka requested a review from TechQuery January 4, 2025 16:50
Comment on lines 142 to 163
export class ActivityAgendaCheckInSummary {
@ViewColumn()
@IsString()
activityId: string;

@ViewColumn()
@IsString()
activityName: string;

@ViewColumn()
@IsString()
agendaId: string;

@ViewColumn()
@IsString()
agendaTitle: string;

@ViewColumn()
@IsInt()
@Min(0)
checkCount: number;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export class ActivityAgendaCheckInSummary {
@ViewColumn()
@IsString()
activityId: string;
@ViewColumn()
@IsString()
activityName: string;
@ViewColumn()
@IsString()
agendaId: string;
@ViewColumn()
@IsString()
agendaTitle: string;
@ViewColumn()
@IsInt()
@Min(0)
checkCount: number;
}
export class ActivityAgendaCheckInSummary extends ActivityCheckInSummary {
@ViewColumn()
@IsString()
agendaId: string;
@ViewColumn()
@IsString()
agendaTitle: string;
}

(上同)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

activity 和 agenda 确实是从属关系,已继承,user 不改动

Copy link
Member

Choose a reason for hiding this comment

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

activity 和 agenda 确实是从属关系,已继承,user 不改动

但也是和 User 参加活动相关的数据呀。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

不一定有相关相同字段就一定要继承吧?如果新加用户关注讲师或者普通用户功能,还要把讲师和活动信息关联上吗?这时候应该是用户关注和打卡继承某个 userBaseOperation ?

Copy link
Member

Choose a reason for hiding this comment

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

不一定有相关相同字段就一定要继承吧?

它们不但有多个相同字段,而且也都是用户跟活动之间的关联数据统计,代码和语义上都有明显的相关性。

如果新加用户关注讲师或者普通用户功能,还要把讲师和活动信息关联上吗?

那肯定就不能了呀,因为你新功能是用户和用户之间的关联数据统计,和活动就没有关系了,语义和字段都不相同。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

所以这里用户打卡不需要强制继承活动打卡统计,他更适合和关注统计一起继承某个 userBaseOperation

Copy link
Member

Choose a reason for hiding this comment

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

所以这里用户打卡不需要强制继承活动打卡统计,他更适合和关注统计一起继承某个 userBaseOperation

首先,我们现在系统中还完全没有“关注”这个功能。

其次,如果以后真有相关功能,软件工程中还有个原则是 —— 不要过早优化。我们现在的优化只针对当前情况,后面如果真有更重复的功数据结构可以继续向上抽基类。

Copy link
Contributor 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.

不能因为有相同字段而给两者加上继承关系

“重复代码段”是发现很多“语义继承关系”的直观基础,这是人脑的直觉。

所以不是我“强加了继承”,是我“发现了继承”。

@Soecka Soecka requested a review from TechQuery January 5, 2025 14:54
@TechQuery TechQuery merged commit 93451bb into master Jan 7, 2025
2 of 3 checks passed
@TechQuery TechQuery deleted the check-event/0103 branch January 7, 2025 14:22
@TechQuery TechQuery self-assigned this Jan 7, 2025
TechQuery added a commit that referenced this pull request Jan 24, 2025
Signed-off-by: TechQuery <shiy2008@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

统计用户活动数据
2 participants