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

102300127 task3 #92

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

dechichichi
Copy link
Contributor

No description provided.

@SchwarzSail
Copy link
Collaborator

如果有做 mit6.824 的话另外再开一个项目,不要和 todolist 放在一起
后续一个 task 一个新的项目仓库

@SchwarzSail
Copy link
Collaborator

对于 todolist 目前做的还不错,不过有以下问题:

  • 有 handler 层,但是 handler 层直接和 dao 进行了交互,缺少了 service 层,dao 层不做业务处理
  • 对于 config 的初始化,可以考虑使用.yaml,这种格式比较通用一些
  • 文档的编写,包含内容为你的项目结构和项目的部署方式。部署方式尽量使用docker形式。也可以写出你项目的特色或者是疑问之类的。
  • 既然使用了 hertz 框架,那么看看 hertz 生成的脚手架,不需要你再手动写 router,只需要在 idl 文件中定义即可。
    有问题的话先 Google,遇到无法理解的问题可以再大胆私聊群里的老登。

@mutezebra
Copy link
Collaborator

102300127 李严

首页

  • README 没有介绍自己项目的内容

项目

按照顺序来

config

  • 使用配置文件来管理项目配置信息非常好. 接下来可以了解一下 yaml 格式, 以及 viper 包
  • 此外, config 中的数据直接用全局变量暴露出来, 是否可以换成一种更优雅的方式? 比如包含在一个结构体里

handler

  • 为什么要把 admin 和 user 分开? 你这也没有管理员模块呀, Login 和 Register 就是用户的功能, 放一起,
  • 这个中间件的使用... 你这还叫中间件吗, 这更像一个包一直在做事情啊, 中间件应该写好逻辑然后注册到路由之后就不用管了, 怎么会不停的在各个handler 中直接调用呢? 是不是理解错了中间件的含义, 总之这个使用方法完全不对, 中间件不是这样用的. 具体可以来找我问, 或者问 ai 或者 google 都可以
  • 没有使用魔法变量, 直接用 http.StatusBadRequest 非常好
  • 额, 有一些 handler 的命名比较奇怪, 跳转到路由表那边发现也有点车马牛不相及, 比如 auth.POST("/:id/task/search", taskhandler.GetTasksToKey) 我不知道 search 和 GetTasksToKey 有什么关系, 你得确保以后的你第一眼看到就懂这是啥吧
  • 还有一点, 不过这个就算进阶内容了. 我发现handler 中会有一些业务级的操作(比如一些 jwt 的调用), 这不是 handler 该做的. 应该交给 service 在你的项目里那个包叫 task.
  • re 到后面回头看突然惊醒, 你的 form-data 呢? 怎么把所有的数据都用 query 来啦? 比如你 task 的数据
	var data model.Data
	err := json.Unmarshal([]byte(c.Query("data")), &data)

query 是从 url 获取数据的. 比如 127.0.0.1:8888/path?id=1234&name=Manu 这里面的你能 query 出来的只有 id = 1234

和 name=Manu, 也就是说你使用 query 传数据的时候数据都是以字符串的形式或者 unicode 的格式放在 url 的, 这样做并不好, 你平时逛网站的时候也不会发现 url 长的离谱吧. 那是因为正常post 的请求的大部分数据都是放在 form-data 中的, 不要放在 query, query 放个 id 啥的差不多了. 正经的数据传递应该用 form-data, 可以了解一下.

middleware

  • 首先有这个包挺好的, 有分离的意识, 但是分离做的并不太好
  • 哦, 终于看懂了, 原来你这个取名叫中间件的包并不只做了中间件, 你还定义了一个全局变量用于解析 token 和颁发 token, 那这样看前面那个操作是没错的.
  • 但是这样也太奇怪了, 一个叫中间件的包在做业务的事情? 不应该, 你或许应该再次分离, 把这个全局变量剥离出来, 放到其他地方. 比如在根目录下开一个 pkg 然后pkg 下开一个 utils uitls 里面开一个 jwt.go 把这个变量相关的操作放这里.

modle

  • 首先有这个模型分离的意识很好, 但是你在这里开数据库连接的时候, 有没有感觉怪怪的, 这是一个叫 模型 的包该做的吗? 或许我们该开另一个包, 比如repository 或者 database,来专门的处理相关的事情

routes

  • 你觉得在根路由下用这个合适吗 h.Use(cache.NewCacheByRequestURI(memoryStore, 2*time.Second)) 或许应该再思考一下他的用途, 我个人觉得很奇怪, 而且感觉会出事的样子
  • 命名问题, 前面没注意到 jwt.MyJwt() , 不好看, jwt 应该放在 middleware 下面, 开一个文件就行了, 而不是一个文件夹, 这样我们 middleware.JWT, 一下子就清晰的知道 jwt 是什么?(中间件), 以及这个中间件的名字(JWT)

task

  • 这个包或许应该叫 service, 你自己写代码的时候会不会搞混? 什么时候 task 是 task 包, 什么时候是 task结构体. 而且 service 才是更适合这个包中文件的名字
  • init.go 里面的数据库初始化, 我已经看到两次了. 为什么会有这么多地方都需要初始化, 还是像我前面说的, 开一个包出来, 就做数据库相关的, 不要到处都是一样的东西
  • 我们一般用一个结构体来管理很多方法, 而不是放很多函数在包里去调用. 比如你可以有一个 UserService 来做这些事情, 还有一个 TaskService. 而不是直接 Register, Login, CreateTask, 你应该懂我的意思
  • 注释很多挺好的, 但是有点太多了,适当的精简会让代码的可读性更好
  • 怎么又是奇怪的命名, TAsk 是什么鬼, 要么就 Task , 要么就 TASK, 什么是 TAsk, 而且这个是有一定规则的, 可以了解一下, 一般缩写短语会全大写
  • 还有这个 Auth. 直接改成 CheckPassword 不就行了, 你这样我会感觉你集成了很多校验机制一样, 但其实就是对比了一下密码. 还是前面所说的, 你要让自己一眼就看得出来这个函数在做什么

main.go

  • 第三次了... 怎么又在初始化数据库? 我仔细看了看, line21 的 sqlDB获取了, 然后呢? 就 defer close? 如果你不需要他的话, 获取他干嘛, 前面line 15 的那个不就够用了.

  • 还有就算你有用, 你这里开了一个 sqlDB 知道 close, 那 config 里面的呢, 那里的那个我没看错的话没关吧, 而且那个毫无作用啊, 没看到任何有引用的地方. 建议精简一下, 不要塞很多东西, 要想清楚哪个模块需要什么? 以及适当的工程思想, 需要复用的东西整合一下呢.

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.

3 participants