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

[WIP] relationdomain refactor ( fork from old PR ) #145

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

xwxb
Copy link

@xwxb xwxb commented Mar 27, 2024

Link to an issue

开了新分支所以重新创建了pr,原来的是这个#138

对应issue:#94

What

在原来基础上主要对 redis 这边做了进一步重构

How

参考 commentDomain 抽象了多个 redis 对象

目前进度

这个重构我拖了很久了,我感觉对我来说还是很有难度的,相较于 userDomaincommentDomain 两个感觉情况都有不一样,留给了我自己太多设计的余地了。
我现在对自己的设计总是因为找不到太多依据所以不是很有自信,可能当初还是有点错误预估难度了,非常抱歉。

所以我还是想就是先发出来想麻烦帮忙看看这样重构是否合理,然后等有集中时间再考虑一下后续设计,讨论好再进行后面的重构。

Checklist

  • Link to an issue if it really related.
  • At least describe what this PR does.
  • Use rebase to confirm that current branch doesn't conflict with main branch.
  • Unit tests. At least do not reduce the unit test coverage.
  • Checked and updated guidelines.md which used to describe how to build, deploy and use DouTok.

Copy link
Owner

@TremblingV5 TremblingV5 left a comment

Choose a reason for hiding this comment

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

有一些小问题可能得改一下,另外需要 @BaiZe1998 来看看

@xwxb
Copy link
Author

xwxb commented Apr 2, 2024

有一些小问题可能得改一下,另外需要 @BaiZe1998 来看看

感谢review,三个问题先改了

@xwxb
Copy link
Author

xwxb commented Apr 21, 2024

不好意思,我可能搞得不是很清楚,目前这个pr是个什么进度?
我之前的想法大概是是想先确定现阶段没问题,然后再考虑后续的设计。

现阶段的重构大概是:

  1. relationDomain 部分的 redis 访问,直接根据具体访问的需要拆成了四个 redis 对象,分别去getset
  2. 创建了一个 总的 relationRepo 对象,用来存所有和db相关的操作。

然后如果现有代码没问题,我就有空接着写了,后面的目标大概是干掉 realtionDomain/service 里最后剩下的大部分面向过程的函数了,主要内容就是继续把repo相关函数都写成 relationRepo 的方法。
我遇到具体问题再来同步,以前主要是不清楚这个重构方向是否有比较大的问题才没法放心往下写。

}

if len(ret) <= 0 {
return ret, redis.Nil
Copy link
Owner

Choose a reason for hiding this comment

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

这里我认为可能没必要返回redis.Nil,这个报错语意应该偏向没有从redis中获取到结果,但这里应该是获取到了结果或是一个空列表

@TremblingV5
Copy link
Owner

不好意思,我可能搞得不是很清楚,目前这个pr是个什么进度? 我之前的想法大概是是想先确定现阶段没问题,然后再考虑后续的设计。

现阶段的重构大概是:

  1. relationDomain 部分的 redis 访问,直接根据具体访问的需要拆成了四个 redis 对象,分别去getset
  2. 创建了一个 总的 relationRepo 对象,用来存所有和db相关的操作。

然后如果现有代码没问题,我就有空接着写了,后面的目标大概是干掉 realtionDomain/service 里最后剩下的大部分面向过程的函数了,主要内容就是继续把repo相关函数都写成 relationRepo 的方法。 我遇到具体问题再来同步,以前主要是不清楚这个重构方向是否有比较大的问题才没法放心往下写。

现有代码我认为整体上没啥大问题。

关于Redis部分

Redis这里的部分代码可能同质性比较高,比如粉丝数/关注数,粉丝列表/关注列表/朋友列表等。这些内容涉及Redis的代码都大差不差,所以我认为可以考虑封装一个方法,用于向Redis存储数字信息或列表信息,然后这几个Redis的代码再去调用。

关于进度

不用担心,慢慢来即可

其他

主分支上现在取消了这种xxxDomain的服务结构,所以建议还是能和主分支同步下,然后把重构的内容添加到relation模块中。

也可以先按你当前的进度搞,后面我帮忙把你的代码调整一下,然后你再继续提起PR

@xwxb
Copy link
Author

xwxb commented Apr 27, 2024

不好意思,我可能搞得不是很清楚,目前这个pr是个什么进度? 我之前的想法大概是是想先确定现阶段没问题,然后再考虑后续的设计。
现阶段的重构大概是:

  1. relationDomain 部分的 redis 访问,直接根据具体访问的需要拆成了四个 redis 对象,分别去getset
  2. 创建了一个 总的 relationRepo 对象,用来存所有和db相关的操作。

然后如果现有代码没问题,我就有空接着写了,后面的目标大概是干掉 realtionDomain/service 里最后剩下的大部分面向过程的函数了,主要内容就是继续把repo相关函数都写成 relationRepo 的方法。 我遇到具体问题再来同步,以前主要是不清楚这个重构方向是否有比较大的问题才没法放心往下写。

现有代码我认为整体上没啥大问题。

关于Redis部分

Redis这里的部分代码可能同质性比较高,比如粉丝数/关注数,粉丝列表/关注列表/朋友列表等。这些内容涉及Redis的代码都大差不差,所以我认为可以考虑封装一个方法,用于向Redis存储数字信息或列表信息,然后这几个Redis的代码再去调用。

关于进度

不用担心,慢慢来即可

其他

主分支上现在取消了这种xxxDomain的服务结构,所以建议还是能和主分支同步下,然后把重构的内容添加到relation模块中。

也可以先按你当前的进度搞,后面我帮忙把你的代码调整一下,然后你再继续提起PR

好的明白,后面有空先看看怎么和主分支同步,然后考虑一下redis这边怎么封装,后面继续写

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