For loop make it reusable javascript

Question

I have some loop like this

for (let i = 0, l = this.documents.length; i < l; ++i) {
  if (item.text !== null) {
    if (this.documents[i].text === item.text) {
      this.documents[i].mandatory = !item.mandatory;
      break;
    }
  } else {
    if (this.documents[i].code === item.code) {
      this.documents[i].mandatory = !item.mandatory;
      break;
    }
  }
}

The problem I have is that I have code smell, with so many if, else, can somebody help me to make it shorter and reusable? Thanks

Hi, please priovide example input and output. Would be best if you could create small snippet. — ulou
– ulou, Commented May 17, 2021 at 8:18
Side note: l = this.documents.length is not a useful optimisation since about IE7 or 8. Browsers and engines should already implement this for you. — VLAZ
– VLAZ, Commented May 17, 2021 at 8:19

georg · Accepted Answer · 2021-05-17 08:47:24Z

1

I'd write it like this:

let doc  = (item.text !== null)
    ? this.documents.find(d => d.text === item.text)
    : this.documents.find(d => d.code === item.code)

 if (doc)
      doc.mandatory = !item.mandatory

answered May 17, 2021 at 8:47

georg

216k57 gold badges324 silver badges401 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Andy · Accepted Answer · 2021-05-17 08:37:58Z

0

You only need one if/else for this

It seems all of those if/else branches are setting the same value. So you only need one if statement.

for (let i = 0, l = this.documents.length; i < l; ++i) {
  if (!item.code && this.documents[i].text === item.text || item.code && this.documents[i].code === item.code) {
    this.documents[i].mandatory = !item.mandatory;
    break;
  }
}

Maybe a little more readable, but a very tiny bit slower in execution:

for (let i = 0, l = this.documents.length; i < l; ++i) {
  var hasItemText = !item.code && this.documents[i].text === item.text;
  var hasItemCode = item.code && this.documents[i].code === item.code;

  if ( hasItemText || hasItemCode ) {
    this.documents[i].mandatory = !item.mandatory;
    break;
  }
}

edited May 17, 2021 at 8:37

Andy

63.6k13 gold badges72 silver badges99 bronze badges

answered May 17, 2021 at 8:25

Peter Pointer

4,2542 gold badges21 silver badges40 bronze badges

Comments

MaatMa · Accepted Answer · 2021-05-17 09:58:52Z

0

Because we don't know what output you expect it's hard to help you. But maybe like this?

const key = item.text ? "text" : "code";
const documentIndex = this.documents.findIndex((cur) => cur[key] === item[key]);
if(documentIndex > -1) this.documents[documentIndex].mandatory = !item.mandatory;

edited May 17, 2021 at 9:58

answered May 17, 2021 at 8:45

MaatMa

2141 silver badge7 bronze badges

5 Comments

loop Over a year ago

The expected output is mutation of the first matching document.

MaatMa Over a year ago

How should you know? You're not the author. The example code is clearly updating the documents array based of the unknown item.

loop Over a year ago

No, the documents array itself stays intact. Only a single property of a single document in the array is updated.

MaatMa Over a year ago

Oh yeah, overread the break vor some reason. But in case the author wants an updated array, I edited my post.

loop Over a year ago

Fails if no matching document is found.

Nina Scholz · Accepted Answer · 2021-05-17 11:33:20Z

0

You could store the key in advance.

Inside the loop, only a single if statement is required to check if a certain value match.

This approach takes the given loop because of unknown this.documents, which could be an array like object. Any other array method, like find needs an array and a conversion to it if not given.

const key = item.text !== null ? 'text' : 'code';

for (let i = 0, l = this.documents.length; i < l; ++i) {
    if (this.documents[i][key] === item[key]) {
        this.documents[i].mandatory = !item.mandatory;
        break;
    }
}

edited May 17, 2021 at 11:33

answered May 17, 2021 at 8:18

Nina Scholz

388k26 gold badges367 silver badges417 bronze badges

1 Comment

Nina Scholz Over a year ago

@loop, sorry i do not understand.

Collectives™ on Stack Overflow

For loop make it reusable javascript

4 Answers 4

Comments

Comments

5 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

Comments

Comments

5 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related