Code review - TypeScript

[srhitect]

Code review - TypeScript (everything mentioned here also applies to JavaScript) stuff

1) extension.ts

const host = (new class {
  isFolderOpen = () => vscode.workspace.workspaceFolders != undefined;
  showInformationMessage = (message : string) => vscode.window.showInformationMessage(message);
});

No need to create anonymous class and immediately instantiate it if you can create object on the spot, moreover it's not idiomatic:

const host = {
  isFolderOpen: () => vscode.workspace.workspaceFolders != undefined,
  showInformationMessage: (message : string) => vscode.window.showInformationMessage(message)
};

Type of the host object that typescript will infer:

host: { isFolderOpen: () => boolean, showInformationMessage: (message: string) => undefined };

2) AllGunControllers.ts, exports/imports

Instead of:

export { GunController };
export { IntelliSenseGunController };
export { CodeLensGunController };
export { SyntaxHighlightingGunController };

You can short it like this:

export { 
  GunController,
  IntelliSenseGunController,
  CodeLensGunController,
  SyntaxHighlightingGunController
}

Or you can do it even shorter with:

export { GunController } from "./GunController";
export { IntelliSenseGunController } from "./IntelliSenseGunController";
export { CodeLensGunController } from "./CodeLensGunController";
export { SyntaxHighlightingGunController } from "./SyntaxHighlightingGunController";

But this requires to remove default keyword from all these files which is ok since default keyword is not really that useful.

3) comparison operators, in ConfigurationDependentGunController.ts, SyntaxHighlightingGunController.ts

I see you use == and != comparison operators which coerce/convert types (if operands are of different type then one of them will be coerced to type of the other so they can be compared). If that is what you wanted to do then ok, but be aware of this behaviour. Alternative is to use comparison operators which don't coerce/convert types: === and !==. Type coercion also happens when doing other stuff (if (x), a + b, ...).

Examples:

[] == true // -> this will evaluate to false because empty list ([]) is coerced into false
[1] == true // -> this will evaluate to true because non-empty list ([1]) is coerced into true

[] === true // -> this will evaluate to false because operands are of different types
[1] === true // -> this will evaluate to false because operands are of different types

Using non-coercive comparison operators is considered good practice in JavaScript/TypeScript world.

4) var keyword, in ConfigurationDependentGunController.ts, demo.ts, index.ts

Keywords var, let and const introduce variable/value bindings, let and const are perfectly fine but var has some serious issues and it should never be used as far as I am concerned.

Variables introduced by let and const are scoped to the nearest {} block, variables introduced by var are scoped to the nearest function body.

Example:

function lousyVar() {
  var x = "var";
  {
    var x = "same var here";
    console.log(x); // prints "same var here"
  }
  console.log(x); // prints "same var here" -> did we really wanted to change outer variable?
}

function niceLet() {
  let x = "let";
  {
    let x = "totally different variable";
    console.log(x); // prints "totally different variable"
  }
  console.log(x); // prints "let" -> outer variable with same name is preserved
}

Everything else seems fine.

[ironcev]

@srhitect Thanks a lot for the review and all the suggestions! I learned a lot out of them and I am looking forward putting all those hints in practice in my future TypeScript coding!