How to convert this code by using find or indexOf?

Question

i created multiple conditions by using map, but i think there are other methods to optimize this code:

 if (content.risks) {
    content.risks.map((risk) => {
      if (risk.coverageCode === "PUB" || risk.coverageCode === "SPPI" || risk.coverageCode === "PROD" || risk.coverageCode === "PR" || risk.coverageCode === "DO") {
        risk._class = "com.generali.gip.local.hk.domain.RiskPublicLiability";
      }
      if (risk.coverageCode === "PD") {
        risk._class = "com.generali.gip.local.hk.domain.RiskMaterialDamage";
      }
      if (risk.coverageCode === "BI") {
        risk._class = "com.generali.gip.local.hk.domain.RiskBusinessInterruption";
      }
    });
  }

How i can to rewrite this code by using find or indexof?

For each would be the way to go and you are already doing that, because you don't return anything in your mapper. Just change the map to forEach and you're done. — Sebastian Speitel, Mar 16 '20 at 08:42
Where did you learn to use `map` where you should be using `for-of` or `forEach`? I see a lot of this, and would really like to know who's teaching this antipattern so I can have a word with them. :-) — T.J. Crowder, Mar 16 '20 at 08:43
@SebastianSpeitel so you dont suggest me to use .find or indexOf? Something like this: ` ["PUB", "SPPI", ....].indexOf(risk.coverageCode) >= 0` ? — Mantegnous, Mar 16 '20 at 08:44
*"How i can to rewrite this code by using find or indexof?"* Neither would be a good choice. You can't use `indexOf` because you're looking for matches based on a property on the object, not the object itself, so it would need to be `find`. Using `find` would mean looping through the array (in `find`) repeatedly. Your current solution loops through once, which is more appropriate. (Just use the right looping construct.) — T.J. Crowder, Mar 16 '20 at 08:45
@T.J.Crowder I'd also like to have a word...It's one of my top pet peeves about programming. — VLAZ, Mar 16 '20 at 08:45
@customcommander what needs optimising, then? Where is the problem and what is the desired outcome? I can't see these details and it's not clear from reading the code what those would be. — VLAZ, Mar 16 '20 at 08:47
@SebastianSpeitel - Yeah, `switch` within `for-of` or `forEach` would be good. Or at least adding the `else`s, but I like `switch` for the above. — T.J. Crowder, Mar 16 '20 at 08:47
To map a value to another value, the simpler approach is to use a map :-) e.g. `{PUB: "com.pub...", PD: "com.pd..."}` — customcommander, Mar 16 '20 at 08:47
@customcommander - That does mean repeating the target value for five of them (`"PUB"`, `"SPPI"`, ...). But also a reasonable approach, perhaps with an actual Map rather than an object... — T.J. Crowder, Mar 16 '20 at 08:49
@T.J.Crowder Yes I was aware of that but in my opinion the "duplication" wouldn't come a the cost of readability (although YMMV of course). All these if statements really get in the way of expressing the _intent_. — customcommander, Mar 16 '20 at 09:10
Note that `.forEach` won't "optimise" anything. It's merely making the code work more correctly. — VLAZ, Mar 16 '20 at 09:14
@VLAZ - Well, `forEach` instead of `map` saves creating a pointless array. It's unlikely to matter, though. :-) — T.J. Crowder, Mar 16 '20 at 10:07
@VLAZ ok so, i have a lot of ||, how i can optimize this? Using an array? Sum? — Mantegnous, Mar 16 '20 at 10:19
@Mantegnous - You've been given lots of suggestions for how to restructure the interior of the loop. `switch`, a lookup object, a lookup Map, ... — T.J. Crowder, Mar 16 '20 at 10:22
@Mantegnous [Check variable equality against a list of values](https://stackoverflow.com/questions/4728144/check-variable-equality-against-a-list-of-values) — VLAZ, Mar 16 '20 at 10:51
@VLAZ can i use `["a","b","C"].some(item => item === opj.field) ` ? — Mantegnous, Mar 16 '20 at 11:19

customcommander · Answer 1 · 2020-03-16T19:05:51.207

My objective in software development is to express my intents in a way that is clear and unambiguous.

For this kind of "problems", I will always reach out for the humble map. There's no extra ceremonies (i.e. if/else, switch/case, etc.). It just says "if you give me an X, I'll give you a Y".

const risk_map =
  { PUB: "com.generali.gip.local.hk.domain.RiskPublicLiability"
  , SPPI: "com.generali.gip.local.hk.domain.RiskPublicLiability"
  , PROD: "com.generali.gip.local.hk.domain.RiskPublicLiability"
  , PR: "com.generali.gip.local.hk.domain.RiskPublicLiability"
  , DO: "com.generali.gip.local.hk.domain.RiskPublicLiability"
  , PD: "com.generali.gip.local.hk.domain.RiskMaterialDamage";
  , BI: "com.generali.gip.local.hk.domain.RiskBusinessInterruption"
  };

Regardless of your programming background, the "time to get it" is low.

It can indeed lead to duplication but I don't think it comes at the expense of readability or maintenance. (How hard would it be to replace these duplicated strings with a constant for example?)

Now how can we "query" this map? Let's build a high-order function for that:

We say "Give me a map, then give me a key, I'll get you a value":

const get_value = map => key => map[key];
get_value(risk_map)("PUB");
//=> "com.generali.gip.local.hk.domain.RiskPublicLiability"

The get_value is a high-order function. We're meant to build specialised functions out of it. Let's just do that:

const get_risk = get_value(risk_map);
get_risk("PUB");
//=> "com.generali.gip.local.hk.domain.RiskPublicLiability"

With this we can solve your "problem" quite simply:

const content_risks =
  content.risks
    ? content.risks.map(risk => get_risk(risk.coverageCode))
    : null;

I'll leave it as an exercise to combine and adapt this to fit your exact needs.

Hope this helps though.

How to convert this code by using find or indexOf?

1 Answers1