如何编写干净的代码而没有所有这些级联错误的圣诞树？

Question

我写了一个应该做简单的事情的函数：

1. 查找表中的特定地址并返回ID，如果已经存在
2. 如果不是，请为此特定地址创建一个新记录
3. 返回此新创建记录的ID

作为RDMS，我在这里使用mysql。我将所有内容放入事务中，以避免在并发的执行该函数的并发例程中出现竞争条件。

但是，对err进行的大量不断检查使代码很难看并且难以获得完整的测试覆盖率。

在这里，我有什么可以改善代码质量的地方吗？

func getAddressId(db *sql.DB, address string) (int64, error) {
  tx, err := db.Begin()
  if err != nil {
    tx.Rollback()
    return 0, err
  }

  stmt, err := tx.Prepare("SELECT id FROM address WHERE `address`=?")
  if err != nil {
    tx.Rollback()
    return 0, err
  }
  defer stmt.Close()

  var result sql.NullInt64
  err = stmt.QueryRow(address).Scan(&result)
  if err != nil && err != sql.ErrNoRows {
    tx.Rollback()
    return 0, err
  }

  if result.Valid {
    tx.Commit()
    return result.Int64, nil
  }

  stmt, err = tx.Prepare("INSERT INTO address (address) VALUES (?)")
  if err != nil {
    tx.Rollback()
    return 0, err
  }

  var res sql.Result = nil
  res, err = stmt.Exec(address)
  if err != nil {
    tx.Rollback()
    return 0, err
  }

  tx.Commit()

  var id int64 = 0
  id, err = res.LastInsertId()

  return id, err
}

Answer 1

首先，也是最重要的是，以上代码没有什么错误。我会调整一些内容（并将在下面进行调整），但总的来说，它非常清楚，直接，并且（几乎）很难出错。这没有什么丑陋的。

其次，关于错误处理Go的想法，请参阅Error Handling and Go，尽管在这里我将不使用那些技术，因为它们不是必需的。

现在有一件事有些不好，那就是很容易忘记在正确的位置调用tx.Rollback()或tx.Commit()。在我看来，这是合理的解决方法（但实际上，它的风格多于实质）。以下未经过测试。

// Name your return values so that we can use bare returns.
func getAddressId(db *sql.DB, address string) (id int64, err error) {
    tx, err := db.Begin()
    if err != nil {
        return // This is a bare return. No need to write "0, err" everywhere.
    }

    // From this point on, if we exit with an error, then rollback, otherwise commit.
    defer func() {
        if err != nil {
            tx.Rollback()
        } else {
            tx.Commit()
        }
    }()

    stmt, err := tx.Prepare("SELECT id FROM address WHERE `address`=?")
    if err != nil {
        return
    }
    defer stmt.Close()  // I'm not sure this is correct, because you reuse stmt

    // This is purely style, but you can tighten up `err = ...; if err` logic like this:
    var result sql.NullInt64
    if err = stmt.QueryRow(address).Scan(&result); err != nil && err != sql.ErrNoRows {
        return
    }

    if result.Valid {
        id = result.Int64
        return
    }

    if stmt, err = tx.Prepare("INSERT INTO address (address) VALUES (?)"); err != nil {
        return
    }

    res, err := stmt.Exec(address)
    if err != nil {
        return
    }

    id = res.LastInsertId()
}

就是说，我认为这个功能做的太多了，如果将其分解，它将变得更易于理解。例如（再次，未经测试）：

func getExistingAddressId(tx *sql.Tx, address string) (id int64, err error) {
    stmt, err := tx.Prepare("SELECT id FROM address WHERE `address`=?")
    if err != nil {
        return
    }
    // I believe you need to close both statements, and splitting it up makes that clearer
    defer stmt.Close()

    var result sql.NullInt64
    if err = stmt.QueryRow(address).Scan(&result); err != nil && err != sql.ErrNoRows {
        return
    }

    // This is probably over-complicated. If !Valid, then .Int64 is 0.
    if result.Valid {
        return result.Int64, nil
    }

    return 0, nil
}

func insertNewAddress(tx *sql.Tx, address string) (id int64, err error) {
    stmt, err := tx.Prepare("INSERT INTO address (address) VALUES (?)")
    if err != nil {
        return
    }
    defer stmt.Close()

    res, err := stmt.Exec(address)
    if err != nil {
        return
    }

    return res.LastInsertId()
}

func getAddressId(db *sql.DB, address string) (id int64, err error) {
    tx, err := db.Begin()
    if err != nil {
        return
    }

    defer func() {
        if err != nil {
            tx.Rollback()
        } else {
            tx.Commit()
        }
    }()

    if id, err = getExistingAddressId(tx, address); err != nil || id != 0 {
        return
    }

    return insertNewAddress(tx, address)
}

使用这样的命名返回值是一个样式问题，您当然不能那样做，而且也很清楚。但是，要点（a）defer是避免重复执行必须始终运行的逻辑的有力方法，而（b）如果函数变得一团糟的错误处理，则可能做得太多。]

作为旁注，我强烈怀疑您可以摆脱这里的Prepare调用，这将大大简化事情。您只能使用一次“语句”。如果您缓存了该语句并重用了它们，那么准备它们就很有意义。如果您这样做，那么代码将简化为：

func getExistingAddressId(tx *sql.Tx, address string) (int64, error) {
    var result sql.NullInt64
    if err := tx.QueryRow("SELECT id FROM address WHERE `address`=?", address).
        Scan(&result); err != nil && err != sql.ErrNoRows {
        return 0, err
    }

    return result.Int64, nil
}

func insertNewAddress(tx *sql.Tx, address string) (int64, error) {
    res, err := tx.Exec("INSERT INTO address (address) VALUES (?)", address)
    if err != nil {
        return 0, err
    }

    return res.LastInsertId()
}

func getAddressId(db *sql.DB, address string) (id int64, err error) {
    tx, err := db.Begin()
    if err != nil {
        return 0, err
    }

    defer func() {
        if err != nil {
            tx.Rollback()
        } else {
            tx.Commit()
        }
    }()

    if id, err = getExistingAddressId(tx, address); err != nil || id != 0 {
        return
    }

    return insertNewAddress(tx, address)
}
而不是尝试简化Go语法，而是简化了操作，其副作用是使语法更简单。

如果您对命名的返回值不太熟悉，可能会忽略其细微之处。在return insertNewAddress(...)中，在id运行之前将函数调用的返回值分配给err和defer，因此if err != nil检查将正确反映返回的值。这可能有点棘手，所以您可能更希望将其编写得更明确，尤其是在函数短得多的情况下。

func getAddressId(db *sql.DB, address string) (int64, error) {
    tx, err := db.Begin()
    if err != nil {
        return 0, err
    }

    var id Int64
    id, err = getExistingAddressId(tx, address)

    if err == nil && id == 0 {
        id, err = insertNewAddress(tx, address)
    }

    if err != nil {
        tx.Rollback()
        return 0, err
    }

    tx.Commit()
    return id, nil
}
现在，代码非常简单，没有任何技巧，而IMO就是最好的选择。