2

So we have quiet a big TypeScript codebase at work, where each method resides in its own file. Now in a lot of those methods the variables are often declared all together at the top/beginnng of the method.

I was discussing with some principal developers that it would be better to declare them only when they're really needed. Because in some cases those variables are used only if some conditions are met. So my point is to avoid executing a bunch of methods to set those variables when they're eventually not needed at all.


Here an example to illustrate my point, keep in mind that this is a very simple example and in some cases we have multiple conditions and more variables.:

Simplified method how it currently is:

namespace some.namespace {

  export async function FunctionName (Toolbar : example.Toolbar) : Promise<void> {

    const Form         : example.Form       = Toolbar.window.getForm("example");
    const WindowCaller : example.Window     = (Toolbar.window.caller as example.Window);
    const DSO          : example.DataSource = WindowCaller.getDataSource("example");
    const ElementID    : number             = DSO.getCurrentRecord()["example"];
    const DynSelect    : example.DynSelect  = Form.getDynSelect("example");
    
    const ExampleEnum      : typeof example.ExampleEnum      = example.ExampleEnum;  
    const OtherExampleEnum : typeof example.OtherExampleEnum = example.OtherExampleEnum;     
    
    if (!Form.validate()) return;

    const Parameter : any = {
      prop_one : ElementID,
      prop_two : DynSelect.value
    };
  
    const Response = await SomeServerCall(Parameter);

    Form.window.close();

    if (Response.someBoolean) {
      DisplayMessage(ExampleEnum.SOMETHING, OtherExampleEnum.SOMETHING );
    }
  };
}

In the example above a bunch of variables are defined and not used before if (!Form.validate()) return; which could stop the execution of the method there. Also the enums are used only in the last if statement.


So what I suggested was to write our code like this, where we declare our variables only when they're actually needed:

My suggestion

namespace some.namespace {

  export async function FunctionName (Toolbar : example.Toolbar) : Promise<void> {

    const Form : example.Form = Toolbar.window.getForm("example");   
    
    if (!Form.validate()) return;

    const WindowCaller : example.Window     = (Toolbar.window.caller as example.Window);
    const DSO          : example.DataSource = WindowCaller.getDataSource("example");
    const ElementID    : number             = DSO.getCurrentRecord()["example"];
    const DynSelect    : example.DynSelect  = Form.getDynSelect("example");

    const Parameter : any = {
      prop_one : ElementID,
      prop_two : DynSelect.value
    };
  
    const Response = await SomeServerCall(Parameter);

    Form.window.close();

    if (Response.someBoolean) {
      const ExampleEnum      : typeof example.ExampleEnum      = example.ExampleEnum;  
      const OtherExampleEnum : typeof example.OtherExampleEnum = example.OtherExampleEnum;  
      DisplayMessage(ExampleEnum.SOMETHING, OtherExampleEnum.SOMETHING );
    }
      
  };
}

Some arguments of the PD's were that the code is less readable like this and if we want to do this we need to add some scopes, for example with an else after the first if. But I don't really see the difference regarding the overall concept of defining variables only if they're needed.


Wouldn't my sggestion be better for performance, memory etc.? Especially when taking into account that some of those early called methods could also be async. Also calling fewer methods means fewer risks that something goes wrong in one of those methods (even if it shouldn't). Also regarding the readability I prefer to see the definitions where they're needed.

So what do you guys think? Am I splitting hairs here?

6
  • 2
    I agree with you: Always declare variables as close to use as possible and in as tight a scope as possible. An early return should also be as early as possible. But your colleagues have a good point that using an if block instead of an early return would create a tighter scope for the variables which is also good style. I usually compromise in the case of early returns since I believe early returns (for validation and other special cases) improve the overall readability. But I acknowledge this is compromise between two principles.
    – JacquesB
    Jun 3 at 17:17
  • 3
    The "declare variables at the top" seems more like a throwback to the times where you had to declare everything at the top.
    – Peter M
    Jun 4 at 14:07
  • 1
    off-topic: this typescript code you are showing is a good example of wrong usage of Typescript. Instead of using interface and infer variable/const types - the code manually annotates each line. The const left: typeof right = right; is ridiculous. const Parameter: any = { a, b } is like WHY WHY WHY ??? Your management should fire your principal developers (or whoever produced this), as they have no idea how to write Typescript (and maybe code in general). As for you - quit and find a place where you can learn from people who can actually code.
    – c69
    Jun 4 at 15:03
  • 1
    It’s worth noting that variables that are not declared at the top are actually “moved” to the top via a process called hoisting
    – John Wu
    Jun 5 at 0:51
  • @c69 I agree that what you mentioned doesn't makes a lot of sense. I have a bit of experience with vanilla JS but not so much with TS and these are topics I also want to address with my colleagues now that I'm getting more experience with TS. Jun 6 at 10:56
9

Generally speaking, I declare variables as closely as possible to their first use, unless I'm using a variable throughout a class instance, in which case I declare it at the top of the class. This essentially eliminates the problem of unused variables and improves code clarity overall.

In this particular case, I (somewhat) favor the first approach; it seems clearer to me (because similar code is being grouped together), and the declaration you (sometimes) save by putting it in the if statement almost certainly isn't going to matter from a performance/memory perspective.

Your biggest win is the single, simple change of moving the Form.Validate call right below the Form declaration, as you have done. That's a good change; if the form doesn't validate, declaring the remaining variables is pointless.

3
  • I largely agree, not going to downvote, but if the code on the right side of the assignments, e.g WindowCaller.getDataSource("example"); is slow or resource intensive, there would be a performance boost to using the second approach.
    – user949300
    Jun 3 at 18:30
  • @Robert Harvey Hi Robert, thx for your answer. I agree that defining the enums in the last if won't have a huge impact. The question isn't specific to this single method but more for a general rule that The variables should only be declared if they're used at runtime. I just used this example to keep it short. I really don't like to execute 5 or 10 methods (even if they're short and quick) for no reason, it feels just wrong to me. Also if they're async it's even worse. Jun 4 at 12:28
  • Also what if the early called methods are designed in the same way? We will then end up with an unpredictable amount of methods called for nothing. Jun 4 at 12:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.