我写了一个应该做简单的事情的函数:
作为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
}
首先,也是最重要的是,以上代码没有什么错误。我会调整一些内容(并将在下面进行调整),但总的来说,它非常清楚,直接,并且(几乎)很难出错。这没有什么丑陋的。
其次,关于错误处理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就是最好的选择。