8
\$\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\$
5
  • \$\begingroup\$ do you have only three types in the array? \$\endgroup\$ Commented Dec 8, 2017 at 8:34
  • \$\begingroup\$ no, the array can have more than 3 types @NinaScholz \$\endgroup\$ Commented Dec 8, 2017 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\$ Commented Dec 8, 2017 at 8:46
  • 2
    \$\begingroup\$ An if/else like that should normally be a switch statement anyway \$\endgroup\$
    – minseong
    Commented Dec 8, 2017 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\$ Commented Dec 8, 2017 at 12:47

6 Answers 6

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\$
4
  • 2
    \$\begingroup\$ Improve is very subjective... I see this and think its just overcomplicating things. Still cool though ;) \$\endgroup\$
    – Ethan
    Commented Dec 8, 2017 at 10:32
  • 3
    \$\begingroup\$ Downvoting this answer since it adds significant complexity and opacity for no measurable benefit. \$\endgroup\$
    – gntskn
    Commented Dec 8, 2017 at 13:52
  • 1
    \$\begingroup\$ This is way too bloated. See Booligoosh's answer, this is the simplest way and is what I would personally have suggested. \$\endgroup\$ Commented Dec 9, 2017 at 16:03
  • \$\begingroup\$ Couldn't you use setAttribute instead of defining 3 functions? \$\endgroup\$ Commented Dec 9, 2017 at 16:28
21
\$\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\$
4
  • \$\begingroup\$ The best and most concise answer here :) \$\endgroup\$
    – Ethan
    Commented Dec 8, 2017 at 10:33
  • 1
    \$\begingroup\$ -1 because it uses the this token which represents a security risk in open environments. \$\endgroup\$
    – Blindman67
    Commented Dec 8, 2017 at 11:16
  • 4
    \$\begingroup\$ @Blindman67, it's not my use of this. and this has nothing to do with a shorter proposal. \$\endgroup\$ Commented Dec 8, 2017 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
    Commented Dec 8, 2017 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\$
6
  • \$\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
    Commented Dec 8, 2017 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
    Commented Dec 8, 2017 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
    Commented Dec 8, 2017 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
    Commented Dec 15, 2017 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
    Commented Dec 15, 2017 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.