8
\$\begingroup\$

I need to create an object based on some properties of another given object. In my example setRules, add properties to object status.

Considering that the cases to be handled will increase increase I would like to figure out how to enhance readability.

I would like to know:

  • Could a switch statement increase readability (please scroll code below to see version 02 of the script)?
  • Any better way to write this from readability point of view?
// version 01
var status = {};
function setRules(cmd) {
    if (cmd.type === 'category' && cmd.action === 'add') {
        status = {
            app: {
                isEdit: false,
                hasPriority: true
            },
            category: {
                lock: false
            }
        };
    } else if (cmd.type === 'category' && cmd.action === 'edit') {
        status = {
            app: {
                isEdit: true,
                hasPriority: true
            },
            category: {
                lock: true
            }
        };
    } else if (cmd.type === 'category' && cmd.action === 'delete') {
        status = {
            app: {
                isEdit: true,
                hasPriority: false
            },
            category: {
                lock: false
            }
        };
    } else if (cmd.type === 'category' && cmd.action === 'read') {
        status = {
            app: {
                isEdit: false,
                hasPriority: false
            },
            category: {
                lock: false
            }
        };
    } else if (cmd.type === 'users' && cmd.action === 'read') {
        status = {
            app: {
                isEdit: false,
                hasPriority: false
            },
            category: {
                lock: false
            }
        };
    } else if (cmd.type === 'users' && cmd.action === 'edit') {
        status = {
            app: {
                isEdit: true,
                hasPriority: true
            },
            category: {
                lock: true
            }
        };
    }
}
setRules({type: 'users', action:'edit'});
// version 02
var status = {};
function setRules(cmd) {
    if(cmd.type === 'category'){
        switch (action) {
            case 'add':
                status = {
                    app: {
                        isEdit: false,
                        hasPriority: true
                    },
                    category: {
                        lock: false
                    }
                };
                break;
            case 'edit':
                status = {
                    app: {
                        isEdit: true,
                        hasPriority: true
                    },
                    category: {
                        lock: true
                    }
                };
                break;
            case 'delete':
                status = {
                    app: {
                        isEdit: true,
                        hasPriority: false
                    },
                    category: {
                        lock: false
                    }
                };
                break;
            case 'read':
                status = {
                    app: {
                        isEdit: false,
                        hasPriority: false
                    },
                    category: {
                        lock: false
                    }
                };
                break;
        }
    } else if (cmd.type === 'users') {
        switch(action){
            case 'read':
                status = {
                    app: {
                        isEdit: false,
                        hasPriority: false
                    },
                    category: {
                        lock: false
                    }
                };
                break;
            case 'edit':
                status = {
                    app: {
                        isEdit: true,
                        hasPriority: true
                    },
                    category: {
                        lock: true
                    }
                };
                break;
        }
    }
}
setRules({type: 'users', action:'edit'});
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Have you considered building status up front and then just setting the boolean values based on your cmd.type and action? I suspect you could make a much shorter and more readable code that way. \$\endgroup\$ Jan 8, 2016 at 10:28
  • \$\begingroup\$ for one, it looks like the cmd.type is not really important. \$\endgroup\$
    – njzk2
    Jan 8, 2016 at 18:15

3 Answers 3

15
\$\begingroup\$

If you know about the format of cmd you can merge the properties you're checking and check against both in one step.

status = and the structure of the Object it's being set to is repeated code, factor it out. When figuring out what to factor out you can almost imagine you're a dumb diff algorithm: what are the minimal text changes necessary to change between these forms? The answer is all the code that should be in that place, plus code to plug it into the thing that implements the commonalities of the form.

Don't be afraid to put code on one line when its complexity is low. Yes you'll need to refactor if the complexity grows, but worrying about possibilities without considering their likelihood is a path to insanity. Make the code look the best it can today.

A rewrite with those changes:

function setRules(cmd) {
  switch (cmd.type + '/' + cmd.action) {
    case 'category/add':    setStatus(false,  true, false); break;
    case 'category/edit':   setStatus( true,  true,  true); break;
    case 'category/delete': setStatus( true, false, false); break;
    case 'category/read':   setStatus(false, false, false); break;
    case 'users/read':      setStatus(false, false, false); break;
    case 'users/edit':      setStatus( true,  true,  true); break;
  }
}

function setStatus(edit, priority, lock) {
  status = {
    app: {isEdit: edit, hasPriority: priority},
    category: {lock: lock},
  };
}
\$\endgroup\$
3
  • \$\begingroup\$ This is very concise, but it still requires the reader to frequently refer back to the definition setStatus(). \$\endgroup\$
    – jelder
    Jan 8, 2016 at 16:58
  • 6
    \$\begingroup\$ If the definition of setStatus is moved away from setRules I would just put a comment above the cases to display the arguments like a table header. \$\endgroup\$
    – agweber
    Jan 8, 2016 at 17:30
  • 4
    \$\begingroup\$ isLock or hasLock for consistency \$\endgroup\$
    – njzk2
    Jan 8, 2016 at 18:16
7
\$\begingroup\$

One option is to use a decision table pattern:

var rules = {
  users: { 
    read   : Status(),
    edit   : Status({isEdit: true})
  }, 
  categories {
    read   : Status({}),
    edit   : Status({isEdit: true, hasPriority: true, lock: true}),
    delete : Status({isEdit: true}),
    add    : Status({hasPriority: true})
  }
};

// sample usage
var status = rules[cmd.type][cmd.caction];

function Status(settings) {
   if(!this) return new Status(settings); // allow without new
   this.app = {};
   this.app.isEdit = settings.isEdit || false;
   this.app.hasPriority = settings.hasPriority || false
   this.category = settings.lock || false;
   Object.freeze(this); // make immutable
   Object.freeze(this.app);
}

By using immutable objects we also don't create a new copy for each element.

I can pretty up the code using ES2015 if you'd like.

Here is a modern version of the above code for Status minus allowing usage with new (per your request):

const Status = ({isEdit = false, hasPriority = false, category = false}) =>
  Object.freeze({ app: Object.freeze({isEdit, hasPriority }), category});
\$\endgroup\$
4
  • \$\begingroup\$ The decision table is acting as a factory by the way, only since objects are immutable all objects (6) are allocated only once and up front - so if in theory you make these choices very often it will likely save you some memory. You also don't have to worry about copying it or it being modified externally. \$\endgroup\$ Jan 8, 2016 at 12:21
  • \$\begingroup\$ Does that "allow without new" trick work in the browser? I imagine this would be window and you end up setting app and category on window before finally "returning" undefined? \$\endgroup\$
    – Supr
    Jan 8, 2016 at 15:06
  • \$\begingroup\$ @Supr good question! I assume strict mode in all code I write and make sure I'm in it. In strict mode this is undefined and not window. I can change that to a this instanceof Status instead perhaps \$\endgroup\$ Jan 8, 2016 at 15:16
  • \$\begingroup\$ Ahh, I see, that makes sense. I'm not too familiar with the specifics of strict mode so I did not realize. TIL. \$\endgroup\$
    – Supr
    Jan 8, 2016 at 15:39
4
\$\begingroup\$

As far as I can tell, there is a logic in your boolean results. If you always apply the same properties for the same action, I'd go with:

function getActionStatus(cmd){
  return {
    app: {
        isEdit: ['edit', 'add', 'delete'].indexOf(cmd.action) !== -1,
        hasPriority: ['edit', 'add'].indexOf(cmd.action) !== -1
    },
    category: {
        lock: ['edit'].indexOf(cmd.action) !== -1
    }
  }
}

If there will be some changes depending on type, and you can't figure out any pattern in your data, you can handle special cases this way:

function getStatus(cmd){
    var specialTypes = {
        user: function(status){
            status.lock = ['whatever', 'other'].indexOf(cmd.action) !== -1
            return status;
        }
    }    

    var status = getActionStatus(cmd);
    if(specialTypes[cmd.type]){
        status = specialTypes[cmd.type](status);
    }
    return status;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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