Which of the following two code snippets do you prefer?

I’m doing research on code quality (this post isn’t part of my research though) i just wanted to get some opinions on two code snippets that do the same thing but are coded differently

Exhibit A:

func (r sqlUserRepo) store(u user) {
	r.upsertUserRecord(u)

	for _, key := range u.keyring {
		if key.isRevoked() {
			r.removeKey(key)
		} else if key.isBumped() {
			r.upsertKeyRecord(key)
		}
	}
}

func (r sqlUserRepo) upsertUserRecord(u user) {
	_, err := r.executor.ExecContext(
		r.ctx,
		`
			INSERT INTO users (id, username, password, email) VALUES ($1, $2, $3, $4)
			ON CONFLICT(id)
			DO UPDATE SET username = $2, password = $3, email = $4
		`,
		u.id,
		u.username,
		u.password,
		u.email,
	)

	panicIfNotNil(err)
}

func (r sqlUserRepo) upsertKeyRecord(key accessKey) {
	_, err := r.executor.ExecContext(
		r.ctx,
		`
			INSERT INTO keys (id, owner, hash, expires_on) VALUES ($1, $2, $3, $4)
			ON CONFLICT(id)
			DO UPDATE SET owner = $2, hash = $3, expires_on = $4
		`,
		key.id,
		key.owner,
		key.hash,
		key.expiresOn,
	)

	panicIfNotNil(err)
}

Exhibit B:

func (r sqlUserRepo) store(u user) {
	_, err := r.executor.ExecContext(
		r.ctx,
		`
			INSERT INTO users (id, username, password, email) VALUES ($1, $2, $3, $4)
			ON CONFLICT(id) DO UPDATE SET username = $2, password = $3, email = $4 WHERE id = $1
		`,
		u.id,
		u.username,
		u.password,
		u.email,
	)
	panicIfNotNil(err)

	for _, key := range u.keyring {
		if key.isRevoked() {
			_, err := r.executor.ExecContext(r.ctx, `DELETE FROM keys WHERE keys.id = $1`, key.id)
			panicIfNotNil(err)
		} else if key.isBumped() {
			_, err = r.executor.ExecContext(
				r.ctx,
				`
				  INSERT INTO keys (id, owner, hash, expires_on) VALUES ($1, $2, $3, $4)
				  ON CONFLICT DO UPDATE SET owner = $2, hash = $3, expires_on = $4
				`,
				key.id,
				key.owner,
				key.hash,
				key.expiresOn,
			)
			panicIfNotNil(err)
		}
	}
}

Can you please read the two code snippets and comment below which one you prefer (found easier to read and understand) and why?, also if you’re working on a code base which one of the two snippets would you rather work on or you would find easier to work with (in other words easier to change)?.

I know this is off topic, but doesn’t your code have a rare race condition if it doesn’t use upsert? UPSERT - PostgreSQL wiki

1 Like

I wasn’t aware that this would’ve caused a race condition (i don’t have a lot of experience with databases), all the statements are executed within a transaction i thought that would’ve been enough to guarantee correct code, thanks for letting me know I’ll fix and update the code.

I am not an expert here. I think if you are using a transaction starting before all statements including the select, and committing after all statements, then you might be okay?

Regardless, the chance of hitting the race condition is rare. I would just add a comment in the code to research upsert later.

To answer which one I’d prefer, I’d need to see more. How are these functions used? I generally like to break steps up into smaller functions, like your first example, however, because all the methods are unexported, how do I know which one (or ones) I should call as a user of the API? store is defined in both samples, so I’m under the impression that that function is the main “entry point” from user code, but without having seen it in the second example, I wouldn’t have known that.

That being said, if users are just supposed to call store and be done with it, I might lean toward example B even though it’s a longer function. From the user’s point of view, a store method on a sqlUserRepo type might be considered a single responsibility. If it’s not, however, and user code might need to know when to upsert a user record or know what these keys are, then maybe they need to remain as separate functions. Maybe there’s code somewhere else like this:

type UserRepo interface {
    store(user)
}

type cachedUserRepo interface {
    upsertUserRecord(user)
    removeKey(accessKey)
    upsertKeyRecord(accessKey)
}

// SaveUser saves the user to the repository.
// if the repo supports accessKeys, the keys are
// cached globally.
func SaveUser(r UserRepo, u user) {
    cu := userFromCache(u)
    tx := beginTx()
    defer func() {
        x := recover()
        if x == nil {
            tx.commit()
            return
        }
        tx.rollback()
        panic(x)
    }()
    switch r := r.(type) {
    case cachedUserRepo:
        r.upsertUserRecord(u)
        for _, k := range u.keyring {
            if k.revoked() {
                decacheAccessKey(k)
                r.removeKey(k)
            } else {
                cacheAccessKey(k)
            }
            if k.isBumped() {
                r.upsertKeyRecord(k)
            }
        }
        return
    }
    r.store(u)
}

Also, you’re using panic to pass errors out of the function which is very unidiomatic in Go. The error should be returned.

Good points, sqlUserRepo is an implementation of an interface called userRepo

interface userRepo {
  store(user)
  load(userQuery) (user, error)
  exists(userQuery) bool
}

type userQuery struct {
  id string
  username string
  email  string
  token  string
}

type sqlUserRepo struct {
  ctx context.Context
  executor persistance.QueryExecutor
}

all this code is inside a small package called identity that basically deals with authentication of users
which is why the store method is not exported because other parts of the app will not use it and will instead use identity.Protect an HTTP middleware that authenticates user requests.

the two other methods in example A are not supposed to be called directly in fact application code will not interact with sqlUserRepo directly but instead will use userRepo so store is the only method publicly visible to the user.

My bad i should’ve included more context in the original post, user structs have a keyring property attached to them, it contains a slice of special access keys these keys are used in the authentication process and have a limited lifespan every time the user uses an access key it gets bumped (the expiry date gets reset) and when the user asks for a new key the newly created key will be bumped too, if the key is revoked (it’s expiration date is past or the user called key.revoke()) then the repo will delete it when it stores the user.

the user object must never be in a bad state (all access keys & other props are valid at creation) the repository abstraction guarantees this in store and load any invalid access keys are deleted when saving or getting a user from the database, load will also panic if the username, email and password don’t pass the user constructor validation.

I agree that panicking just because a query failed might not be the best idea but i think it’s okay to call panic if app detects corruption in the database.

1 Like