current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

Here I'm instantiating different classes based on the value of a type property. This type property will determine what view is rendered and displayed on screen to the user.

As you can see there's a lot of repetition here. The main problem I'd like to eliminate is all those else ifs. 

onShow: function() {
    var type = this.model.get('type').toLowerCase();

    if (type === 'face') {
        this.regionItems.show(new FaceDetail({
            model               : this.model,
            authRequestModel    : this.authRequestModel,
            collection          : this.model.attemptItemsDataCollection
        }));
    } else if (type === 'voice') {
        this.regionItems.show(new VoiceDetail({
            model               : this.model,
            authRequestModel    : this.authRequestModel,
            collection          : this.model.attemptItemsDataCollection
        }));
    } else if (type === 'text-prompted') {
        this.regionItems.show(new VoiceDetail({
            model               : this.model,
            authRequestModel    : this.authRequestModel,
            collection          : this.model.attemptItemsDataCollection
        }));

    } else if (type === 'face-password (text)') {
        this.regionItems.show(new PinDetail({
            model               : this.model,
            authRequestModel    : this.authRequestModel,
            collection          : this.model.attemptItemsDataCollection
        }));

    } else if (type === 'pin') {
        this.regionItems.show(new PinDetail({
            model               : this.model,
            authRequestModel    : this.authRequestModel,
            collection          : this.model.attemptItemsDataCollection
        }));
    }  else if (type === 'live voice') {
        this.regionItems.show(new VoiceLiveness({
            model               : this.model,
            authRequestModel    : this.authRequestModel,
            collection          : this.model.attemptItemsDataCollection
        }));
    } else if (type === 'voice-face (image)') {
        this.regionItems.show(new FaceDetail({
            model               : this.model,
            authRequestModel    : this.authRequestModel,
            collection          : this.model.attemptItemsDataCollection
        }));
    } else if (type === 'voice-face (audio)') {
        this.regionItems.show(new VoiceDetail({
            model               : this.model,
            authRequestModel    : this.authRequestModel,
            collection          : this.model.attemptItemsDataCollection
        }));
    } else if (type === 'gps') {
        this.regionItems.show(new GPSDetail({
            model               : this.model,
            authRequestModel    : this.authRequestModel,
            collection          : this.model.attemptItemsDataCollection
        }));

    } else if (type === 'face-password (image)') {
        this.regionItems.show(new FaceDetail({
            model               : this.model,
            authRequestModel    : this.authRequestModel,
            collection          : this.model.attemptItemsDataCollection
        }));
    } else if (type === 'voice-face (challenge audio)') {
        this.regionItems.show(new VoiceLiveness({
            model               : this.model,
            authRequestModel    : this.authRequestModel,
            collection          : this.model.attemptItemsDataCollection
        }));
    } else if (type === 'fingerprint') {
        this.regionItems.show(new FingerDetail({
            model               : this.model,
            authRequestModel    : this.authRequestModel,
            collection          : this.model.attemptItemsDataCollection
        }));
    }
}

I intend to extract

{
    model               : this.model,
    authRequestModel    : this.authRequestModel,
    collection          : this.model.attemptItemsDataCollection
}

to the top of the method as an options variable and then pass it to the constructor when I instantiate each view (FaceDetail, VoiceDetail, etc.)

After this I'm still left with all those else ifs.

share|improve this question
1  
please use the title to tell us what your code is doing, rather than what you are looking for from a review of your code. thank you –  Lyle's Mug yesterday
    
Ok, thanks - editing now –  garethdn yesterday

1 Answer 1

up vote 8 down vote accepted

The obvious answer is to introduce a map:

var typeToConstructor = {
    'face' : FaceDetail,
    'voice' : VoiceDetail,
    // ...
}

Then you can simply look up your constructor function:

onShow: function() {
    var type = this.model.get('type').toLowerCase();
    var constructor = typeToConstructor[type];
    if (typeof constructor !== 'function') {
        // Unknown type... do something
    }

    // execute the constructor.
    this.regionItems.show(new constructor({
        model               : this.model,
        authRequestModel    : this.authRequestModel,
        collection          : this.model.attemptItemsDataCollection
    });
}
share|improve this answer
    
I was considering this but there is a slightly awkward situation where multiple types map to the same class. e.g. both voice and text-prompted map to the VoiceDetail class. I know that this doesn't rule out the "mapping" object at all but I thought it would be good if there was a way to map each class to multiple types, without repeating the class name....if that makes sense. –  garethdn yesterday
    
Yes that makes sense, it's one of those times where you have to put the mapping in somewhere and it's usually going to be horrible looking. Just realised I wrote "obvious" when I really meant "superficial". –  RobH yesterday
    
To be fair, a mapping object is pretty much the "obvious" answer –  garethdn yesterday
    
For what it's worth, it's completely fine and proper to have the same value appear multiple times in a map. –  Ben Collier 10 hours ago

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.