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

Question

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

在 table 和 return ID 中查找特定地址，如果已经存在
如果没有，请为此特定地址创建一个新记录
return这条新建记录的ID

作为 RDMS，我在这里使用 mysql。我将所有内容都放在一个事务中，以避免在调用此函数的并发 go-routines 中出现竞争条件。

然而，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

首先，也是最重要的一点，上面的代码几乎没有错误。有几部分我会调整（并将在下面），但通常它非常清晰、直接，并且（几乎）很难出错。这没什么不好的。

其次，请参阅 Error Handling and Go 了解有关 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)
}

像这样使用命名的 return 值是一种风格问题，您当然不能那样做，但它会同样清晰。但要点 (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 值不是很熟悉，可能会忽略一个小细节。在 return insertNewAddress(...) 中，函数调用的 return 值在 defer 运行之前分配给 id 和 err，因此 if err != nil 检查将正确反映 returned 值。这可能有点棘手，因此您可能更愿意将其全部写得更明确，尤其是现在函数要短得多。

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 是 Go 的最佳状态。

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

How to write clean code without all these cascading error Christmas trees?

code-cleanup

go