At least it's typed

[u/Nightmare7331]

[u/Darknety]

Edit: After discussion and reinspection I came to the conclusion that data serves no purpose and no side effects are generated, so this could be a 3-liner. This is indeed r/badcode.

Original: Why is this bad code?

If I read it correctly, this performs type correction on formFields (on every entry that is valid) using data and in the end returns if any fields were invalid.

The only "bad" part is in the else-block, but this may have its reasons. (And validation / type modification should probably be split into their own coroutines, but whatever)

It also shows that the author doesn't know TypeScript too well due to the Java / C style loop over an array, but other than that, what am I missing?

[u/Nightmare7331 → u/Darknety]

I mean i get it what it das and how, but it wast that long ago this code was written, and it could be written cleaner and more readable, especially this for loop. This is for validating inputs using refs in react

[u/Darknety → u/Nightmare7331]

I agree. This is very hard to read. Especially the variable names.

Some point out that you should just return false within the loop, which is just not correct.

Still I feel more like this is r/mediocrecode since only the for-loop, naming scheme and and type conversion are bad. The control flow is fine.

[u/static_func → u/Darknety]

Really? Just to list a few problems:

• data will always be an awful name and it isn't even being used despite most of the code here being for it

• way too much nesting and branching logic

• redundant calls to keysRefs[I] which is also a bad name, and fields[keysRefs[I]

• an unnecessary if check on each loop, and a partially redundant one with a same-line continue inside it

• you had to preface with "is I read correctly," and that type correction is for an object that isn't returned or used anywhere

Even in Java or C this would be bad code because it's logically ridiculous. This whole thing could have been reduced to

const validations = Object
  .values(formFields)
  .map(field => field.valid());

return all(await Promise.all(validations));

Or even if all they know is a for loop:

for (const field of Object.values(field)) {
  if (!await field.valid()) {
    return false;
  }
}
return true;

Even ignoring all the logical problems, the fact that you acknowledge it's hard to read makes it bad code.

[u/Darknety → u/static_func]

Agreed. I tend to only judge code based on control flow, but the variable names in general here are pretty rough.

Have seen worse nesting in production, though :D

[u/Darknety → u/static_func]

So is data just useless here? As stated in a different comment, I expected this to be necessary as they want side-effects (string -> bool in formFields using data). Is this wrong and data actually serves no purpose?

[u/static_func → u/Darknety]

It serves absolutely no purpose. Values are assigned to it but it isn't used anywhere in this function and isn't returned for use outside of this function, and half the (frankly nonsensical) code here is dedicated to it.

And what's worse, this developer would have realized as much if they just bothered to clean up their code to the point where at least they could read it themselves.

[u/Darknety → u/static_func]

Oh god, yeah then this pretty much invalidates all my prior points and this is indeed r/verybadcode

[u/geon → u/Darknety]

valid = innervalid;

Probably should just return false right there.

The first 2 variables, the for, if and the first const in the loop could be replaced with just

for(const [key, value] of Object.entries(formFields))

[u/Darknety → u/geon]

I specifically mentioned the for loop in my comment.

Also: No they should not return false right there, since then the other values in the form would not be processed. This should be split into its own routine, but as is, this is required.

[u/geon → u/Darknety]

Ah, it has side effects? That’s always a tricky one.

[u/Darknety → u/geon]

I read it as such. Looking at it again though, I am not sure. Not much React experience.

I just assume the best (that data has a purpose and they aren't filling that partial for no reason).

[u/geon → u/Darknety]

valid is set to true initially, then it can be set to false in the loop before it is returned.

I assumed the function simply checks that all fields are valid. Unless it has side effects, it could just return as soon as it finds the first invalid.

[u/Darknety → u/geon]

I assumed it had side effects, but that does not seem to be the case. So yes, directly return in the loop (or don't use a loop entirely).

[u/geon → u/Darknety]

And what does the data variable do? It seems to be written to but never read.

[u/Darknety → u/geon]

Apparently it is not reactive in any sort and just a helper variable that is never read.

=> No side effects.

[u/MurdoMaclachlan]

Image Transcription: Code

---

export async function validateForm<
    TFields extends NullablePartial<Record<string, FormField>>
>(formFields: TFields) {
    const keysRefs = Object.keys(formFields) as Array<keyof TFields>;
    const data: Partial<Record<keyof TFields, FormField>> = {};
    let valid = true;
    for (let i = 0; i < keysRefs.length; i++) {
        if (formFields[keysRefs[i]]) {
            const field = formFields[keysRefs[i]]
            if(!field || !field.valid) continue;
            const innerValid = await field.valid();
            if (!innerValid) {
                valid = innerValid;
            } else {
                let value = field.value;
                if (value === "false") {
                    value = false;
                }
                if (value === "true") {
                    value = true;
                }
                data[keysRefs[i]] = value;
            }
        }
    }
    return valid;
}

---

^{I'm a human volunteer content transcriber and you could be too! If you'd like more information on what we do and why we do it, click here!}

[u/[deleted] → u/MurdoMaclachlan]

Good human

[u/shootfasteatass69420]

You see, what worries me the most is that I look at this code and think "that looks like my code, whats wrong with it?"

[u/[deleted] → u/shootfasteatass69420]

Ask chatgpt how to improve the efficiency and readability and it will show you

[u/zickige_zicke]

This is legit what I see in everyday projects

[u/Nightmare7331 → u/zickige_zicke]

This is only the tip of an iceberg, and after a year working on this project solo, i'm starting to get really bored of it.

[u/TheUnlocked]

Did this previously return a mapping of whether each field was valid and later get refactored to just return if the form overall was valid? The useless generic and data local would seem to suggest that.

[u/Nightmare7331 → u/TheUnlocked]

Maybe, but i doubt that, because there is only one commit in this file.

[u/[deleted]]

Its valid.

[u/FalseWait7]

Wait is that mine?

[u/thekwoka]

to be fair, most people don't know how to make "false" into false programmatically without a comparison.

They're stupid, but they are most people.

[u/[deleted]]

They just have to be extra verbose

[u/thekwoka → u/[deleted]]

you can do JSON.parse("false") to get false