记录一下这段时间的Code Review

前言

前段时间参与的基本都是倒排需求，小伙伴写得比较急，可能有些地方代码写的时候没考虑太清楚。然后我在CR的时候，就发现了一些坑和一些小问题，抽空记录一下

有些问题没有对错之分，只是用来形成最大共识以及共同审美

1. 判断数据是否已存在

// SELECT * FROM `user` WHERE conditions...
func (u *UserRepo) CheckExist(ctx context.Context, conditions ...gen.Condition) (bool, error) {
   users, err := dao.User.WithContext(ctx).Where(conditions...).Find()
   ...
   return len(users) != 0, nil
}

有什么问题:

• 主要是性能问题。业务逻辑是想判断是否已经有存在的数据了，并不需要把符合条件的数据都查出来

怎么解决:

// 基于InnoDB引擎

// 查到返回true，查不到返回false
SELECT EXISTS (SELECT * FROM `user` WHERE conditions...)

// 查到返回1，查不到什么都不返回
SELECT 1 FROM `user` WHERE conditions... limit 1

// 查到返回匹配的行数，查不到返回0
SELECT count(*) FROM `user` WHERE conditions...

查询的条件记得要命中索引，然后具体case具体分析，用之前最好先用explain分析一下

2.复杂的判断需要抽取出一个可读的函数

// 目的：比较code与公司的QualificationCode和RegNum是否不相同
func BadCase() {
    if len(company.QualificationCode) > 0 && 
        code != company.QualificationCode && 
        (len(company.RegNum) > 0 && code != company.RegNum || len(company.RegNum) == 0) {
     // doSomething()...
    }
}

有什么问题呢:

• 可读性极差，不利于理解
• 认知复杂度极高

怎么解决呢：

不断拆分出可读的判断方法

func GoodCase() {
    if company.IsNotTheSameCode(exceptCode) {
        // doSomething()...
    }
}
func (c *Company) IsNotTheSameCode(code string) bool {
    return !c.IsTheSameCode(code)
}

func (c *Company) IsTheSameCode(code string) bool {
    return c.IsTheSameQualificationCode(code) || c.IsTheSameRegNumCode(code)
}

func (c *Company) IsTheSameQualificationCode(code string) bool {
    return len(c.QualificationCode) > 0 && code == c.QualificationCode
}

func (c *Company) IsTheSameRegNumCode(code string) bool {
    return len(c.RegNum) > 0 && code == c.RegNum
}

• 可读可复用

3. 充血模式，收敛逻辑

func BadCase() {
    // 判断用户状态
    if user.Status == constants.Deleted || user.Status == contants.Unauthorized {
        // doSomething()...
    }
    ...
    // 转成其他
    userDTO := &UserDTO{
        ID:     user.GetID(),
        Name:   user.GetName(),
        Email:  user.GetEmail(),
        Status: user.GetStatus(),
    }
    ....
}

有什么问题：

• 代码逻辑松散，较难复用

较好的写法：

func GoodCase() {
    if IsNormalUser() {
        // doSomething()...
    }
    ....
    userDTO := ToDto(user)
 }

 // 将可复用的逻辑以函数的方式抽取出来
 // 相应的逻辑都集中、收敛在函数中
 func IsNormalUser() bool {
     return user.Status != constants.Deleted && 
     user.Status != contants.Unauthorized
 }

  func IsAbnormalUser() bool {
      return !IsNormalUser()
  }

  func ToDto(user *User) *UserDTO {
      return &UserDTO{
            ID:     user.GetID(),
            Name:   user.GetName(),
            Email:  user.GetEmail(),
            Status: user.GetStatus(),
        }
  }

• 这些可复用的逻辑照理来说应该会被多处引用
  • 那么这些函数应该放在哪个目录、哪个类里面呢
  • 怎么让别人知道你已经封装了相应的函数，而不至于又重写了一遍类似的逻辑呢

充血模式：

func GoodCase() {
    if IsNormalUser() {
        // doSomething()...
    }
    ....
    userDTO := ToDto(user)
 }

 ---------

 // 将可复用的逻辑以方法的方式抽取出来
 // 相应的逻辑都集中、收敛在User结构体的文件中
 // 因为以方法的形式封装逻辑，所以可以通过User对象快速找到相应的逻辑
func (u *User) IsNormalUser() bool {
    return u.Status != constants.Deleted && 
        u.Status != contants.Unauthorized
}

func (u *User) IsAbnormalUser() bool {
    return !u.IsNormalUser()
}

func (u *User) ToDto() *UserDTO {
      return &UserDTO{
        ID:     u.GetID(),
        Name:   u.GetName(),
        Email:  u.GetEmail(),
        Status: u.GetStatus(),
      }
}

• 相应的逻辑都集中、收敛在User结构体的文件中
• 因为以方法的形式封装逻辑，所以可以通过User对象快速找到相应的逻辑

4. 一行代码较为合适的长度

func BadCase(ctx context.Context, request Request){
    userDetails, err := repo.UserRepo.GetUsersByCharacteristic(ctx, request.GetHeight(), request.GetWeight(), request.GetAge(), request.GetGender())
}

func GetUsersByCharacteristic(ctx context.Context, gender int, age int, height float64, weight float64) (users []*User, err error) {

}

有什么问题:

• 主要是可读性问题

怎样才比较好：

• golint提示控制字符长度在120以内
• 不要超过编辑器的右边界竖线

怎么解决:

1. （go语言）同样的类型可以共用一个类型标识

// gender和age共用int类型
// height和weight共用float64
// gender int, age int, height float64, weight float64 变成下面的
// gender, age int, height, weight float64
func GetUsersByCharacteristic(ctx context.Context, gender, age int, height, weight float64) (users []*User, err error) {
    ...
}

2. 变量名称可以用具有特定语意的简写

// 例如 request可以简写成req
func Case(ctx context.Context, req Request){
    userDetails, err := repo.UserRepo.GetUsersByCharacteristic(ctx, req.GetHeight(), req.GetWeight(), req.GetAge(), req.GetGender())
    ...
}

3. 提取出可以复用的逻辑

// 假设下面还用到请求参数
// UserRepo也会用到
func GoodCase(ctx context.Context, req Request){
    height, weight := req.GetHeight(), req.GetWeight()
    userRepo := repo.UserRepo
    userDetails, err := userRepo.GetUsersByCharacteristic(ctx, height, weight, req.GetAge(), req.GetGender())
    ...
}

4. 实在不行就换行吧

5. time.Time的相关踩坑

func BadCase(){
    // 1. 解析时间字符串
    getUpTimeStr := "2022-10-31 09:00:00"
    getUpTime, _ := time.Parse("2006-01-02 15:04:05", getUpTimeStr)
    now := time.Now()   // "2022-10-31 09:00:01"
    fmt.println(time.Now().After(getUpTime)) // false
    
    // 2. 获取月底的时间 
    // 获取上个月底的时间 2022-09-30 23:59:59 
    larstDayOfLastMonth := time.Date(getUpTime.Year(), getUpTime.Month(), 0, 23, 59, 59, 0, time.Local) 
    larstDayOfThisMonth := larstDayOfLastMonth.Add(0, 1, 0)
    fmt.println(endOfThisMonthTime) // 2022-10-30 23:59:59
    
    // 3. 获取下个月的时间
    // getUpTime 2022-10-31 09:00:00
    nextMonthTime := getUpTime.AddDate(0, 1, 0) // AddDate(years, months, days int)
    fmt.println(nextMonthTime) // 2022-12-01 09:00:00
}

什么原因导致的呢:

1. time.Parse默认使用UTC时区去解析时间，当跟机器上的时区不一致时，会出现不符合预期的场景
2. 9月的月底时30号，所以10月去获取上个月的月底时间，只能拿到9月30日, 增加一个月份，就变成了10月30日
3. 原因跟上类似，11月份是没有31号的，所以当用AddDate()去增加2022-10-31的月份时，会变成2022-12-01，也不符合预期

怎么搞

func GoodCase(){
    // 1. 解析时间字符串
    getUpTimeStr := "2022-10-31 09:00:00"
    // 使用ParseInLocation代替Parse
    getUpTime, _ := time.ParseInLocation("2006-01-02 15:04:05", getUpTimeStr, time,Location)
    now := time.Now()   // "2022-10-31 09:00:01"
    fmt.println(time.Now().After(getUpTime)) // true
    
    // 2. 获取月底的时间
    // 以当月的1号为基准
    firstDayOfThisMonth := time.Date(now.Year(), now.Month(), 1, 0, 0, 0, 0, time.Local)
    larstDayOfThisMonth := firstDayOfThisMonth.AddDate(0, 1, -1)
    fmt.println(firstDayOfThisMonth) // 2022-10-31 23:59:59
    
    // 3. 获取下个月的时间
    // 具体情况按业务分析吧，（下个月的时间）到底是下个月的对应日期，还是直接加31天
}

• 使用ParseInLocation代替Parse去解析日期字符串
• 以1号为基准去做对应月份的加减
• 具体情况具体分析
• 多写单测，多注意边界场景

6. for range循环操作副本变量

func BadCase() {
    UserMap := map[int]string{
       1: "XiaoMing",
       2: "XiaoQiang",
    }
    userNameList := []*string{}
    for _, name := range UserMap {
       userNameList = append(userNameList, &name)
    }
    for i := range userNameList {
       fmt.Println(*userNameList[i])  // XiaoQiang  XiaoQiang
    }    
}

什么原因呢:

• 现象：循环中获取的变量，它们的地址都是一样的
• 本质：for range会把每一次遍历到的值，赋给同一个临时变量（这个临时变量从始至终的内存地址都是不变的，变的只是它的内容）
  • 遍历map时的伪代码
  • 遍历slice/Array的伪代码
  • 遍历时如何转换变量的源代码

怎么解决呢:

func GoodCase() {
    UserMap := map[int]string{
       1: "XiaoMing",
       2: "XiaoQiang",
    }
    userNameList := []*string{}
    for _, name := range UserMap {
       tempName := name 
       userNameList = append(userNameList, &tempName)
    }
   ------------
   for userNum := range UserMap {
       name := UserMap[userNum]
       userNameList = append(userNameList, &name)
    }
}

• 不应该直接使用返回变量的内存地址，可以先赋值个一个临时变量tempName，再用它的内存地址
• 通过操作原数组来获取需要的变量。如UserMap[userNum]

7. 业务字段尽可能不使用零值

Add COLUMN `user_type` tinyint NOT NULL DEFAULT '0' COMMENT '用户类型，0:普通用户，1:VIP用户'

type UserType int

const (
   UserCommon UserType = 0
   UserVIP    UserType = 1
)

有什么问题呢:

• 业务含义与零值耦合在一起了，当这个字段是0时，区分不出是没有赋值，还是代表业务含义
• 一些组件有时候会忽略传进来的零值。如gorm的Updates()

怎么搞呢:

// 区分开零值与业务枚举
Add COLUMN `user_type` tinyint NOT NULL DEFAULT '0' COMMENT '用户类型，1:普通用户，2:VIP用户'

type UserType int

const (
   Unknown UserType = 0
   Common  UserType = 1
   VIP     UserType = 2
)
---------
// 最近看DDD的时候还发现了一种有趣的写法，贴一贴
type UserType struct {
    value int
}
var (
   Unknown  = UserType{0}
   Common   = UserType{1}
   VIP      = UserType{2}
)
var userTypeValues = []UserType{
   Common,
   VIP,
}
func NewUserType(value int) UserType {
    for _, item := range userTypeValues {
       if item.value == value {
          return item
       }
    }
    return Unknown
}

8. 对外透露的ID不要采用自增

有什么问题呢：

• 不安全，容易让用户知道ID的规律
• 如果生产环境和测试环境是共用同一个下游服务，还可能会导致下游服务的数据混乱
  • 例如，共用下游的日志服务，测试环境把ID为1的数据记录在了日志服务上，那么生产环境ID为1的数据在这个日志服务上能查到测试环境的书

怎么解决呢：

• 使用ID生成器来生成所需的ID

9. 浮点数的相关操作

func BadCase() {
    // 直接相加
    a, b := 2.3329, 3.1234
    c := a + b // c => 5.456300000000001

    // 直接相乘
    a = 1128.61
    b = a * 100  // b => 112860.99999999999
}

什么原因呢：

• IEEE 754
• 0.1 + 0.2不等于0.3的相关讨论
• 浮点运算

怎么解决：

func GoodCase(){
    // "github.com/shopspring/decimal" 
    a := decimal.NewFromFloat(2.3329) 
    b := decimal.NewFromFloat(3.1234) 
    c := a.Mul(b)
}

• 使用Decimal包来操作浮点数

10. 判断字符长度

func BadCase() {
    str := "今晚几点下班?"
    fmt.Println(len(str)) // 19
}

为什么会这样:

• func len(v Type) int的注释 String: the number of bytes in v.，由于Go的字符串都以UTF-8编码的，因此每个中文占3个字节，所以就是6*3+1 = 19

怎么解决:

• 如果字符串包含中文，可以使用utf8.RuneCountInString()

11. 不要在循环里操作数据库/调用RPC

有什么问题:

• 主要是性能问题
• 其次，还有可能造成死循环的风险

怎么解决呢:

• 循环操作改成批量查询

12. Slice的相关操作

func BadCase() {
    nameList := make([]string, 0)             // 1. 这样初始化不太好
    if userList != nil && len(userList) > 0 { // 2. 判断Slice是否为空可以直接用len()
        for _, user := range userList {       //    或者直接使用for range语法糖
            nameList = append(nameList, user.Name)
        }
    }
}

有什么问题呢：

• 初始化Slice

  • 当不确定Slice的长度时
    • nameList := []string{} 初始化成一个empty的slice，会序列化成 []，不推荐。因为在初始化时会分配一个内存，写入数据时很可能需要重新分配内存
    • var nameList []string 一般使用这种初始化，用法大部分和上面一致，初始化nil比一个empty slice更省空间。 但是会序列化成null
  • 当确定了Slice的长度时, 使用 make(t type, len int, cap int) 初始化

• 判断slice是否为空，可以直接使用len(userList)或者for _, user := range userList

如示：

func GoodCase() {
    var nameList []string
    for _, user := range userList {
         nameList = append(nameList, user.Name)
    }
}