7
\$\begingroup\$

I have this loop that iterates and assigns a variable to true depending on the different conditions

for (const element of actionsReferences) {
  if (element === 'accept') {
    this.showAcceptButton = true
  } else if (element === 'reject') {
    this.showRejectButton = true
  } else if (element === 'transfer') {
    this.showTransferButton = true
  }
}

How can i get the same result by avoiding if () ?

\$\endgroup\$

We are looking for answers that provide insightful observations about the code in the question. Answers that consist of independent solutions with no justification do not constitute a code review, and may be removed.

migrated from stackoverflow.com Dec 8 '17 at 8:35

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ do you have only three types in the array? \$\endgroup\$ – Nina Scholz Dec 8 '17 at 8:34
  • \$\begingroup\$ no, the array can have more than 3 types @NinaScholz \$\endgroup\$ – Mouad Ennaciri Dec 8 '17 at 8:37
  • 4
    \$\begingroup\$ Several showXButton variables indicate a deeper code stink. You might want to show more of your code so that the core issue can be fixed. \$\endgroup\$ – Guy Incognito Dec 8 '17 at 8:46
  • 2
    \$\begingroup\$ An if/else like that should normally be a switch statement anyway \$\endgroup\$ – theonlygusti Dec 8 '17 at 9:05
  • \$\begingroup\$ Your question has been migrated from Stack Overflow to Code Review. Here, we advise you to show some more code, so that we see exactly what this code is for and give you the best advice possible. Perhaps we could even eliminate these assignments altogether? See How to Ask. \$\endgroup\$ – 200_success Dec 8 '17 at 12:47
5
\$\begingroup\$

You could use a string to function "map", in JavaScript that can be implemented with a simple object:

var map = {
  'accept'   : function(o) {  o.showAcceptButton = true; },
  'reject'   : function(o) {  o.showRejectButton = true; },
  'transfer' : function(o) {  o.showTransferButton = true; }
};

let thisObject = {}; // fake this object

map['accept'](thisObject);
map[element](this); // use within your loop


// ES6 map
const map6 = {
    accept   : (o) => o.showAcceptButton = true,
    reject   : (o) => o.showRejectButton = true,
    transfer : (o) => o.showTransferButton = true
};

// alternative ES6 map
const map6a = {
    accept(o)   { o.showAcceptButton = true; },
    reject(o)   { o.showRejectButton = true; },
    transfer(o) { o.showTransferButton = true; }
};

map6['reject'](thisObject);
map6a['transfer'](thisObject);

// check if function exists and really is a function
if ('accept' in map6 && typeof map6['accept'] === 'function') map6['accept'](thisObject);
\$\endgroup\$
  • 2
    \$\begingroup\$ Improve is very subjective... I see this and think its just overcomplicating things. Still cool though ;) \$\endgroup\$ – Ethan Dec 8 '17 at 10:32
  • 3
    \$\begingroup\$ Downvoting this answer since it adds significant complexity and opacity for no measurable benefit. \$\endgroup\$ – gntskn Dec 8 '17 at 13:52
  • \$\begingroup\$ This is way too bloated. See Booligoosh's answer, this is the simplest way and is what I would personally have suggested. \$\endgroup\$ – Micheal Johnson Dec 9 '17 at 16:03
  • \$\begingroup\$ Couldn't you use setAttribute instead of defining 3 functions? \$\endgroup\$ – Eric Duminil Dec 9 '17 at 16:28
19
\$\begingroup\$

You could take an object and check if the action exists. If so, take the value as key for assignment.

const actions = {
        accept: 'showAcceptButton',
        reject: 'showRejectButton',
        transfer: 'showTransferButton'
    };

for (const element of actionsReferences) {
    if (element in actions) {
        this[actions[element]] = true;
    }
}
\$\endgroup\$
  • \$\begingroup\$ The best and most concise answer here :) \$\endgroup\$ – Ethan Dec 8 '17 at 10:33
  • 1
    \$\begingroup\$ -1 because it uses the this token which represents a security risk in open environments. \$\endgroup\$ – Blindman67 Dec 8 '17 at 11:16
  • 3
    \$\begingroup\$ @Blindman67, it's not my use of this. and this has nothing to do with a shorter proposal. \$\endgroup\$ – Nina Scholz Dec 8 '17 at 11:17
  • \$\begingroup\$ I am well aware it is not your use this I think your exelent answer is not as exelent an answer as the accepted answer. Your answer is not as flexible as the functional approch and I want votes to go to the better answer. What if on of the buttons required an additional action. Code already in place is usually adapted in light of new requirements rather than re-written which would degrade the quality of you code. The functional approch can handle unique actions without addition logic. \$\endgroup\$ – Blindman67 Dec 8 '17 at 12:28
5
\$\begingroup\$

You can simply use this code:

for (const element of actionsReferences) {
  this.showAcceptButton = element === 'accept';
  this.showRejectButton = element === 'reject';
  this.showTransferButton = element === 'transfer';
}

Or, if you want the variables to stay as what they were set to if it returns false, use this:

for (const element of actionsReferences) {
  this.showAcceptButton = element === 'accept' || this.showAcceptButton;
  this.showRejectButton = element === 'reject' || this.showRejectButton;
  this.showTransferButton = element === 'transfer' || this.showTransferButton;
}
\$\endgroup\$
  • \$\begingroup\$ Was about to add this answer. The one thing I'd change is to add brackets to make it quicker for human to parse. this.showAcceptButton = (element === 'accept'); \$\endgroup\$ – Peter Dec 8 '17 at 12:02
  • \$\begingroup\$ The first part of this answer is probably the best representation of what OP wanted in this entire thread; the second part could be improved by using || rather than a ternary: this.showAcceptButton = element === 'accept' || this.showAcceptButton \$\endgroup\$ – gntskn Dec 8 '17 at 13:55
  • \$\begingroup\$ The first snippet is wrong because only at most one button will be visible in the end. The second example contains an obvious code smell, as already pointed out in the comment above. \$\endgroup\$ – xehpuk Dec 8 '17 at 15:51
  • 1
    \$\begingroup\$ @Carl these are not if else branches, they're === and || operations. The difference being that a) they do not accept any blocks, and are therefore a simpler and more primitive language structure and b) the compiler can (to some extent, since we're dynamic here) simply use CMP and OR instructions, which are simple math operations that do not require any branching at all. \$\endgroup\$ – gntskn Dec 15 '17 at 10:30
  • 1
    \$\begingroup\$ @Carl I now see that this comment was left before the answer was updated to use || over ternaries; I'll leave my comment in case anyone happens to have my imagined criticism, however :p \$\endgroup\$ – gntskn Dec 15 '17 at 10:32
0
\$\begingroup\$

You can use bracket notation.

You have to capitalize first letter of element also.

element.charAt(0).toUpperCase() + element.slice(1)

Solution

for (const element of actionsReferences) {
   this['show' + element.charAt(0).toUpperCase() + element.slice(1) + 'Button'] = true
}
\$\endgroup\$
0
\$\begingroup\$

Try bracket notation

var fnToFirstUpper = (str) => str.charAt(0).toUpperCase() + str.substring(1);

for (const element of actionsReferences) {
  this["show" + fnToFirstUpper( element ) + "Button" ] = true;
}
\$\endgroup\$
0
\$\begingroup\$

You can use indexOf to achieve this:

var actionsReferences = ['accept', 'reject', 'transfer']
var showAcceptButton = actionsReferences.indexOf('accept') > -1;
var showRejectButton = actionsReferences.indexOf('reject')> -1;
var showTransferButton = actionsReferences.indexOf('transfer')> -1;

console.log(showAcceptButton);
console.log(showRejectButton);
console.log(showTransferButton);

\$\endgroup\$

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