tslint complaining "statements must be filtered with an if statement" when using switch

Question

Lets say I have the following method:

getErrorMessage(state: any, thingName?: string) {
    const thing: string = state.path || thingName;
    const messages: string[] = [];
    if (state.errors) {
        for (const errorName in state.errors) {
            switch (errorName) {
                case 'required':
                    messages.push(`You must enter a ${thing}`);
                    break;
                case 'minlength':
                    messages.push(`A ${thing} must be at least ${state.errors['minlength'].requiredLength}characters`);
                    break;
                case 'pattern':
                    messages.push(`The ${thing} contains illegal characters`);
                    break;
                case 'validateCardNumberWithAlgo':
                    messages.push(`Card doesnt pass algo`);
                    break;
            }
        }
    }
    return messages;
}

when I run

ng lint

I get the following error :

for (... in ...) statements must be filtered with an if statement

Having a look at similar question I don't think that answer would be applicable to my situation. After all switch statement sits in the category of if-else-if ladder.

tslint should consider switch statement as form of if statement but it doesn't?!

Balling

use if else instead of switch? – Actualize

Would you suggest the same thing if I had 100 conditions to check? – Balling

I would actually suggest you refactor the inner for loop into something else. :) A method call, or maybe map over the errors array to create the list of error messages. :) – Actualize

Possible duplicate of What does the JSLint error 'body of a for in should be wrapped in an if statement' mean? – Actualize

check out that question, it's for JS but the same applies. It has nothing to do with the switch statement, but with the for .. in :) – Actualize

@Actualize I think the point is that the rule says that an if statement should come after the for...in, and a switch statement can be seen as a kind of if statement, yet it doesn't resolve the error. – Traitor

I believe the point of the rule is to filter out any properties that come from the prototype chain of the object accidentally. i.e. it expects if (obj.hasOwnProperty(propName)). Right now you cannot be sure that required exists directly on state.errors object or from a prototype of it. – Venge

@Venge you are correct - that is the intention of the rule. But you can literally have any if statement and the error will go away - it doesn't check specifically for hasOwnProperty. So in that case, a switch statement could be seen as similar to any if statement. The case statements could even be targeting only the property names known not to be on the prototype. – Traitor

@FrankModica you are right, maybe the switch statement is not really used for checking if an object has a property. :-/ Anyway I added an answer that gets by this rule by not using for .. in at all. :) – Actualize

Answer 1

This made me curious, so I checked out the TSlint source code for this rule. It has a function called isFiltered which seems to only check for ts.SyntaxKind.IfStatement, not for ts.SyntaxKind.SwitchStatement.

function isFiltered({statements}: ts.Block): boolean {
    switch (statements.length) {
        case 0: return true;
        case 1: return statements[0].kind === ts.SyntaxKind.IfStatement;
        default:
            return statements[0].kind === ts.SyntaxKind.IfStatement && nodeIsContinue((statements[0] as ts.IfStatement).thenStatement);
    }

}

So unless you want to convert your object to an array, you'll need to use a fix from the link you provided. Either Object.keys, or an if statement:

    for (const errorName in state.errors) {
      if (state.errors.hasOwnProperty(errorName)) {
        switch (errorName) {

The interesting thing is that you can have any kind of if statement and the error will go away. There is no check to see if you are calling hasOwnProperty.

Traitor

Answer 2

The rule is meant to prevent you from accessing properties defined on the object prototype when using for .. in.

You can however refactor the code to just not use that, and make it easier to maintain and develop as well.

An example would be this:

interface ErrorMessageFactory {
  (thing: string, state?): string
}

type Errors = 'required' | 'minlength' | 'pattern' | 'validateCardNumberWithAlgo'

let errorFactory: {[e in Errors]: ErrorMessageFactory} = {
  required: (thing) => `You must enter a ${thing}`,
  minlength: (thing, state) => `A ${thing} must be at least ${state.errors['minlength'].requiredLength}characters`,
  pattern: (thing) => `The ${thing} contains illegal characters`,
  validateCardNumberWithAlgo: (thing) => `Card doesnt pass algo`
}



function getErrorMessage(state: any, thingName?: string) {
  if (state.errors) {
    return state.errors.map((error) => errorFactory[error](thingName, state));
  }
  return [];
}

You can see a working snippet in the playground here.

Actualize