current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 6 down vote favorite
1

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'});
share|improve this question
2  
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. – SuperBiasedMan 23 hours ago
    
for one, it looks like the cmd.type is not really important. – njzk2 16 hours ago
up vote 13 down vote

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},
  };
}
share|improve this answer
    
This is very concise, but it still requires the reader to frequently refer back to the definition setStatus(). – jelder 17 hours ago
4  
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. – agweber 16 hours ago
3  
isLock or hasLock for consistency – njzk2 16 hours ago
up vote 6 down vote

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});
share|improve this answer
    
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. – Benjamin Gruenbaum 22 hours ago
    
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? – Supr 19 hours ago
    
@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 – Benjamin Gruenbaum 19 hours ago
    
Ahh, I see, that makes sense. I'm not too familiar with the specifics of strict mode so I did not realize. TIL. – Supr 18 hours ago
up vote 2 down vote

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;
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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