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)?.