Repository: valkey-io/valkey
File: src/t_string.c (C)

Code before:
                       robj *val,
                       robj *expire,
                       int unit,
                       robj *ok_reply,
                       robj *abort_reply,
                       robj *comparison) {
    long long milliseconds = 0; /* initialized to avoid any harmness warning */
    int found = 0;
    int setkey_flags = 0;

    if (expire && getExpireMillisecondsOrReply(c, expire, flags, unit, &milliseconds) != C_OK) {
        return;
    }

    if (flags & OBJ_SET_GET) {
        if (getGenericCommand(c) == C_ERR) return;
    }

    found = (lookupKeyWrite(c->db, key) != NULL);

    /* Handle the IFEQ conditional check */
    if (flags & OBJ_SET_IFEQ && found) {
        robj *current_value = lookupKeyRead(c->db, key);

        if (current_value == NULL || current_value->type != OBJ_STRING || checkType(c, comparison, OBJ_STRING) != 0) {
            if (!(flags & OBJ_SET_GET)) {
                addReplyError(c, "value(s) must be present or string");
            }
            return;
        }

        if (compareStringObjects(current_value, comparison) != 0) {
            if (!(flags & OBJ_SET_GET)) {
                addReply(c, abort_reply ? abort_reply : shared.null[c->resp]);
            }
            return;
        }

    } else if (flags & OBJ_SET_IFEQ && !found) {
        if (!(flags & OBJ_SET_GET)) {
            addReply(c, abort_reply ? abort_reply : shared.null[c->resp]);
        }
        return;
    }

    if ((flags & OBJ_SET_NX && found) || (flags & OBJ_SET_XX && !found)) {
        if (!(flags & OBJ_SET_GET)) {
            addReply(c, abort_reply ? abort_reply : shared.null[c->resp]);
        }
        return;
    }

Review comment (refactor):
In this block `found` is true, so I believe we don't need to check `current_value` as NULL. We don't support key with `NULL` value in Valkey.
Also, `comparison` object is from the client input, so I think we can be assured it would be of string type. We should use the checkType method call for `current_value`. We can simplify it to:

```suggestion
        if (checkType(c, current_value, OBJ_STRING)) {
            return;
        }
```

Code after:
                       robj *val,
                       robj *expire,
                       int unit,
                       robj *ok_reply,
                       robj *abort_reply,
                       robj *comparison) {
    long long milliseconds = 0; /* initialized to avoid any harmness warning */
    int found = 0;
    int setkey_flags = 0;

    if (expire && getExpireMillisecondsOrReply(c, expire, flags, unit, &milliseconds) != C_OK) {
        return;
    }

    if (flags & OBJ_SET_GET) {
        if (getGenericCommand(c) == C_ERR) return;
    }

    robj *existing_value = lookupKeyWrite(c->db, key);
    found = existing_value != NULL;

    /* Handle the IFEQ conditional check */
    if (flags & OBJ_SET_IFEQ && found) {
        if (!(flags & OBJ_SET_GET) && checkType(c, existing_value, OBJ_STRING)) {
            return;
        }

        if (compareStringObjects(existing_value, comparison) != 0) {
            if (!(flags & OBJ_SET_GET)) {
                addReply(c, abort_reply ? abort_reply : shared.null[c->resp]);
            }
            return;
        }
    } else if (flags & OBJ_SET_IFEQ && !found) {
        if (!(flags & OBJ_SET_GET)) {
            addReply(c, abort_reply ? abort_reply : shared.null[c->resp]);
        }
        return;
    }

    if ((flags & OBJ_SET_NX && found) || (flags & OBJ_SET_XX && !found)) {
        if (!(flags & OBJ_SET_GET)) {
            addReply(c, abort_reply ? abort_reply : shared.null[c->resp]);
        }
        return;
    }

    /* If the `milliseconds` have expired, then we don't need to set it into the
     * database, and then wait for the active expire to delete it, it is wasteful.
     * If the key already exists, delete it. */
    if (expire && checkAlreadyExpired(milliseconds)) {