2
\$\begingroup\$

I've got this massive nested for loop with a few if and else statements in nodeJs. The output of all of this is just string which is combined from different values out of JSon object. What I want to do is rewrite it in more efficient way as JSlint plugin for sublime is complaining about complexity of it:

var instancesList = '';
    ec2.describeInstances(params,function(err, data) {
        if (err){ 
            console.log(err, err.stack);
        } else {
                for (var i in data.Reservations){
                    for (var j in data.Reservations[i].Instances[0].Tags){
                        if(data.Reservations[i].Instances[0].Tags[j].Key == 'Name'){
                            instancesList+= 'Name: ' + data.Reservations[i].Instances[0].Tags[j].Value +
                                           '  id: ' +data.Reservations[i].Instances[0].InstanceId +
                                           '  Status: ' + data.Reservations[i].Instances[0].State.Name +'\n';   
                        }    
                    }
                }
        }
console.log(instancesList);

Any help, ideas or a link on how to deal with these kind of situations would be greatly appreciated.

\$\endgroup\$
  • \$\begingroup\$ Hi! Welcome to Code Review. Good job on your first post! \$\endgroup\$ – TheCoffeeCup Nov 16 '15 at 23:46
  • \$\begingroup\$ Please declare your cross-posts in the future. \$\endgroup\$ – 200_success Nov 16 '15 at 23:56
1
\$\begingroup\$

Honestly, your loop doesn't seem that complex. I would probably ignore this warning in your case, at least until it grew in complexity or the surrounding function grew in size.

In any case, here are some things you could change:

a. Add a variable to replace the five instances of data.Reservations[i].Instances[0].

b. Off the top of my head, I'm pretty sure an EC2 instance is only going to have one "Name" tag. You can extract this loop into a separate function if you feel comfortable with this assumption.

c. If your lint rules / personal style allows for multiple returns, you can reduce the depth by returning from the error.

var instancesList = '';
ec2.describeInstances(params,function(err, data) {
  if (err) return console.log(err, err.stack);

  for (var i in data.Reservations){
    var ins = data.Reservations[i].Instances[0]
    var name = findInstanceName(ins)
    if(name === null) continue;

    instancesList+= 'Name: ' + name +
                    '  id: ' +ins.InstanceId +
                    '  Status: ' + ins.State.Name +'\n';
  }
  console.log(instancesList);
}

function findInstanceName(ins){
  for(var j in ins.Tags){
    if(ins.Tags[j].Key === 'Name'){
      return ins.Tags[j].Value;
    }
  }
  //I'm not sure skipping the instance is the best choice.
  //You might instead return "<Unknown>" so you can know you
  //have an unnamed instance running.
  return null;
}

Brief aside: If this script is only to list the instances for you, you might find it useful to check out the aws-cli and the jq tool:

aws ec2 describe-instances | jq -r '.Reservations[].Instances[] | [(.Tags | from_entries | .Name), .InstanceId, .State.Name] | @tsv' | column -t -s $'\t'

Output:

hello-world      i-a1a1a1a1  running
jenkins          i-b2b2b2b2  running
production       i-c3c3c3c3  running
qa               i-d4d4d4d4  stopped
testing          i-e5e5e5e5  terminated
\$\endgroup\$

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.