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.

Currently I have

var l1 = [...];
var l2 = [...]; 
if (l1.length > l2.length){
    var i = l2.indexOf(l1[0]);
    if (i !== -1){
        action_a(i);
    }else{
        action_c();
    }
} else if (l1.length < l2.length){
    var i = l1.indexOf(l2[0]);
    if (i !== -1){
        action_b(i);
    }else{
        action_c();
    }
} else {
    action_c();
}

Which I have could refactor into the following

function foo() {
    var l1 = [...];
    var l2 = [...];
    var i = -1;
    if (l1.length > l2.length){
        i = l2.indexOf(l1[0]);
    } else if (l1.length < l2.length){
        i = l1.indexOf(l2[0]);
    }
    if (i === -1 || l1.length == l2.length) {
        action_c();
        return;
    }
    if (l1.length > l2.length){
        action_a(i);
    } else if (l1.length < l2.length){
        action_b(i);
    }
}

It's a bit better, but I'm checking for the length of the two array twice. It's also still not as readable as I would like it to be.

Can you review the above code and offer recommendations on how to do this better?

share|improve this question
    
Is there a typo on the 4th line of the original code? Should l[0] be l1[0]? The same typo seems to be in the refactored code as well. –  GraniteRobert Jun 17 at 17:56
    
Yea that is a typo, I will fix it –  XrXrXr Jun 17 at 18:00

3 Answers 3

up vote 2 down vote accepted

After re-reading the question I realized my answer was the same as your original code. Which frankly I don't see a problem with (the upper one, the later one is worse).

It has a clear structure which says "Okay, I'm treating three cases: less, equals and greater and doing different things in each case.

If you want to get rid of the duplication of calls to action_c you can do like so:

var l1 = [...];
var l2 = [...]; 
if (l1.length > l2.length){
    var i = l2.indexOf(l1[0]);
    if (i !== -1){
        action_a(i);
        return;
    }
} else if (l1.length < l2.length){
    var i = l1.indexOf(l2[0]);
    if (i !== -1){
        action_b(i);
        return;
    }
} 
action_c();

But I would argue it's less readable as it's not immediately obvious when action_c is called as it is in the original. But in this case it's splitting hairs IMHO.

edit: corrected variable name

share|improve this answer
    
The problem with the first fragment is that I have action_c() scattered all over the tree, which might be fine, but it doesn't feel right –  XrXrXr Jun 12 at 14:03
    
Edited for how to get rid of action_c being everywhere. Although I don't think it's a problem that it is, it makes it easier to analyse. –  Emily L. Jun 12 at 14:06
    
Fair enough, I think I will just stick to the original –  XrXrXr Jun 12 at 14:06
    
Thanks for accepting the answer, but as you can see from the lack of up-votes on my answer you might want to wait a bit and see if some one else has some other tips. –  Emily L. Jun 12 at 14:09
    
Good call, I guess I will come back later –  XrXrXr Jun 12 at 14:10
var l1 = [...];
var l2 = [...]; 
if (l1.length > l2.length){
    actOnDuplicateOfFirstElementIfPresent(l1, l2, action_a);
} else if (l1.length < l2.length){
    actOnDuplicateOfFirstElementIfPresent(l2, l1, action_b);
} else {
    action_c();
}

function actOnDuplicateOfFirstElementIfPresent(la, lb, action) {
    var i = lb.indexOf(la[0]);
    if (i !== -1){
        action(i);
    }else{
        action_c();
    }
}

This Extract Method refactoring eliminates the duplication found in the first two branches. Eliminating duplication is both an excellent motivation, and an excellent technique, for refactoring.

share|improve this answer
    
thank you for your answer, but would you please explain the changes that you made and why you made them? –  Malachi Jun 12 at 14:16
    
I saw duplication in the first two branches, so I extracted the method. Eliminating duplication is one of the best techniques and motivations for refactoring. –  Carl Manaster Jun 12 at 14:18
    
Factoring into more functions is always an option. But in this particular case I feel that the call to foo hides the notion that action_c() acts as a kind of "fallback" if the other conditions fail. But then again it all depends on the semantics of the three functions and their relations. –  Emily L. Jun 12 at 14:18
    
I think that's only a problem because of the name "foo". Not knowing anything else about this code it was impossible to come up with a meaningful name. –  Carl Manaster Jun 12 at 14:20
    
@CarlManaster, I was meaning to add definition to your code inside your answer post, Code only answers are frowned upon in much of the StackExchange Network. –  Malachi Jun 12 at 14:25

I noticed that some of the variables didn't add up l and l1 I assumed that l is supposed to be l1 in the code that I wrote.

I moved the actions into their respective places because I couldn't see a reason that you couldn't merge them before checking for the lengths being equal or i being unassigned.

function foo() {
    var l1 = [...];
    var l2 = [...];
    var i = -1;

    if (l1.length > l2.length){
        i = l2.indexOf(l1[0]);
        action_a(i);
    } else if (l1.length < l2.length){
        i = l1.indexOf(l2[0]);
         action_b(i);
    } else if (i === -1 || l1.length == l2.length) {
        action_c();
    }
}

From here I can see that the i variable is something that you don't really need, so I go a step further

function foo() {
    var l1 = [...];
    var l2 = [...];

    if (l1.length > l2.length){
        action_a(l2.indexOf(l1[0]));
    } else if (l1.length < l2.length){
         action_b(l1.indexOf(l2[0]));
    } else if (l1.length == l2.length) {
        action_c();
    }
}

I think this is what you want.


upon further reading it appears that you want an else statement to perform action_c(); if one is not greater than the other or something else happens, so you could perform one less check at the end

function foo() {
    var l1 = [...];
    var l2 = [...];

    if (l1.length > l2.length){
        action_a(l2.indexOf(l1[0]));
    } else if (l1.length < l2.length){
         action_b(l1.indexOf(l2[0]));
    } else {
        action_c();
    }
}

I realize that I left out a check for either Array being empty, so I remedy this like:

function foo() {
    var l1 = [...];
    var l2 = [...];


    if (l1.length > l2.length  && l2.length > 0){
        action_a(l2.indexOf(l1[0]));
    } else if (l1.length < l2.length && l1.length > 0){
         action_b(l1.indexOf(l2[0]));
    } else {
        action_c();
    }
}

I left out a check for if the match is not found so I have added a couple of ternaries

function foo() {
    var l1 = [...];
    var l2 = [...];


    if (l1.length > l2.length  && l2.length > 0){
        l2.indexOf(l1[0]) ? action_a(l2.indexOf(l1[0])) : action_c();
    } else if (l1.length < l2.length && l1.length > 0){
         l1.indexOf(l2[0]) ? action_b(l1.indexOf(l2[0])) : action_c();
    } else {
        action_c();
    }
}
share|improve this answer
1  
You're missing the case where the indexOf operator doesn't find the element in the list and returns -1 which I assume is an invalid argument to action_a and action_b. So your code is not functionally equivalent. –  Emily L. Jun 12 at 14:50
    
@EmilyL. now it does, thank you for pointing that out. –  Malachi Jun 12 at 15:03
    
Looking at the last edit, its essentially the original code, with the detriment of using indexOf twice. Also, if both arrays are empty, it will fall through to the else block anyways, adding an if is just more checks. (both arrays are length of 5, it first check if both are empty then check twice to find their length are equal) –  XrXrXr Jun 12 at 15:09
    
@XrXrXr if either is empty then it will stop at the first or second check, if they aren't empty there is a possible match, so we check for that. still thinking about the indexOf function though... this code is much cleaner and clearer though –  Malachi Jun 12 at 15:14
    
indexOf would return -1 anyways when either array is empty, so existing early doesn't really save much, since it adds overhead when both are not. There is also the problem of indexOf returning 0, which is false-ish to the ternary, resulting in a faulty call to action_c() –  XrXrXr Jun 12 at 15:18

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.