Handle switch case statements without a break #84

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
@nilbus
Contributor

nilbus commented Sep 15, 2011

Problems

This valid javascript produces this error: TypeError: Cannot call method 'isA' of undefined

switch(letter) {
    case 'a': // note the lack of any statement here
    case 'b':
        a_or_b();
        break;
    case 'c':
        c();
        break;
}

Case statements with no break should run subsequent case statements until a break is reached.

switch(letter) {
    case 'a':
        a_only();
    case 'b':
        a_and_b();
        break;
}

This code javascript compiles into this coffeescript, which fails to run a_and_b() when letter is 'a':

switch letter
  when "a"
    a_only()
  when "b"
    a_and_b()

Proposed solution

In the last example, when letter is 'a', both a_only() and a_and_b() should be called. There are a couple ways I could have gone here:

Redundancy:

switch letter
  when "a"
    a_only()
    a_and_b()
  when "b"
    a_and_b()

Storing functions to call:

_cases =
  "a": =>
    a_only()
  "b": =>
    a_and_b()
switch letter
  when "a"
    _cases["a"]();
    _cases["b"]();
  when "b"
    _cases["b"]();

The former will be awkward when switch case statements use many chained cases (no break) and have lots of code. The latter really isn't too bad, but I don't feel confident about a few things, such as _cases not overwriting real variables, and actually coding this in js2coffee. This is my first time looking at the project.

That said, I decided to go with the former for now. Let me know if you disagree with that approach.

Testing

Tests have been updated and pass. Notably, the returns test was updated to include break statements to avoid redundantly testing this feature since it is tested by the switch test.

nilbus added some commits Sep 15, 2011

Handle switch case statements without a break
The returns test was updated to include break statements to avoid redundantly
testing this feature, since it is tested by the switch test.
@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Sep 20, 2011

Contributor

big -1, this patch fails in numerous ways with regards to flow control

Contributor

michaelficarra commented Sep 20, 2011

big -1, this patch fails in numerous ways with regards to flow control

@nilbus

This comment has been minimized.

Show comment
Hide comment
@nilbus

nilbus Sep 21, 2011

Contributor

I only recently was aware of #76, which I think more elegantly solves this problem. Even though it doesn't handle normal fall-through cases yet, I like the method @chills42 suggested using a conditional block. Much better than duplicating the code.

@michaelficarra Could you clarify what you meant about control flow though?

Contributor

nilbus commented Sep 21, 2011

I only recently was aware of #76, which I think more elegantly solves this problem. Even though it doesn't handle normal fall-through cases yet, I like the method @chills42 suggested using a conditional block. Much better than duplicating the code.

@michaelficarra Could you clarify what you meant about control flow though?

@nilbus nilbus closed this Sep 21, 2011

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Sep 21, 2011

Contributor

#91 pointed out the fact that http://phpjs.org/functions/version_compare:852 would not compile properly. Use your fork to compile it. The output is horribly ugly and, worse yet, incorrect. The return is ignored and the following statements are tacked on anyway.

Contributor

michaelficarra commented Sep 21, 2011

#91 pointed out the fact that http://phpjs.org/functions/version_compare:852 would not compile properly. Use your fork to compile it. The output is horribly ugly and, worse yet, incorrect. The return is ignored and the following statements are tacked on anyway.

@nilbus

This comment has been minimized.

Show comment
Hide comment
@nilbus

nilbus Sep 21, 2011

Contributor

Thanks for testing that. I should have tested it on something more complex like the script you linked.

Contributor

nilbus commented Sep 21, 2011

Thanks for testing that. I should have tested it on something more complex like the script you linked.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Sep 21, 2011

Contributor

No problem, I love testing things out and reviewing code.

Contributor

michaelficarra commented Sep 21, 2011

No problem, I love testing things out and reviewing code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment