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

Question

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

1. 在表中查找特定地址并返回 ID（如果已存在）
2. 如果没有，则为此特定地址创建一条新记录
3. 返回此新创建的记录的 ID

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

然而，大量的持续检查使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，了解 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）如果函数变得混乱的错误处理，则它可能做得太多了。

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

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分配，因此检查将正确反映返回值。这可能有点棘手，因此您可能更愿意更明确地编写这一切，尤其是现在该函数要短得多。errdeferif 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
}

现在代码非常简单，没有任何技巧，在我看来，这是 Go 的最佳状态。