Как написать чистый код без всех этих каскадных рождественских елок ошибок?

Я написал функцию, которая должна делать простую вещь:

  1. искать определенный адрес в таблице и возвращать идентификатор, если он уже существует
  2. если нет, создайте новую запись для этого конкретного адреса
  3. вернуть идентификатор этой вновь созданной записи

В качестве 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
}

person delete    schedule 29.02.2020    source источник
comment
Для откатов и коммитов: ищите defer. Для ошибок: так они обрабатываются в Go.   -  person mkopriva    schedule 29.02.2020
comment
Я согласен, что рождественские елки ошибок уродливы. Однако модульные тесты имеют цель проверить, тестируете ли вы все ветки. Если ветвь покрывает возможность сбоя, вы должны протестировать ее.   -  person Heap Underflow    schedule 29.02.2020
comment
См. также: stackoverflow.com/questions/16963298/ и blog.golang.org/errors-are-values   -  person mkopriva    schedule 29.02.2020
comment
Этот стиль является тщательным и гарантирует корректность кода.   -  person colm.anseo    schedule 29.02.2020


Ответы (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)
}

Использование именованных возвращаемых значений, как это, является вопросом стиля, и вы, конечно, не могли бы сделать это таким образом, и это было бы так же ясно. Но пункт (а) defer — это мощный способ избежать дублирования логики, которая должна выполняться всегда, и (б) если функция становится беспорядочной при обработке ошибок, возможно, она делает слишком много.

В качестве примечания, я сильно подозреваю, что вы могли бы избавиться от вызовов 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
}

И теперь код очень простой, без уловок, что, по моему мнению, лучше всего подходит для Go.

person Rob Napier    schedule 29.02.2020