Use switch statement in chatHandler.observe()

RESOLVED WONTFIX

Status

defect
RESOLVED WONTFIX
7 years ago
7 years ago

People

(Reporter: tetsuharu, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Use switch statement for clean up.
Attachment #611150 - Flags: review?(florian)
(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #1)
> Created attachment 611150 [details] [diff] [review]
> proposed patch

Personally I find this much more difficult to read (especially with the increased tab levels). But I can't review things in mail/ so I'll leave this for Florian. :)
I don't see a compelling reason to take this change (I wouldn't say it's "much more difficult to read", but it doesn't really feel like an improvement either, except for the single case where 4 different values are handled by the same code), so if we took this on comm-central I wouldn't see any reason for requesting comm-aurora-approval on it, and that would make any fix touching this code in comm-central twice as painful to backport to aurora.
Attachment #611150 - Flags: review?(florian)
Resolving to WONTFIX (feel free to reopen if you see a really good reason for us to want this change :)).
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WONTFIX
I think that using switch statement increases maintainability of this method.
chatHandler.observe() handle calling methods which topics correspond to.
Using switch statement is betther rather than if & return on this purpose.
The correspondence relation of methods and topics line up straight if we take switch statement.
So this increases readability and maintainability.

And I don't think this patch land to comm-aurora.
This patch is maked with simple replacement, so I'll make easy the backport.
But this patch does not fix a concrete bug for usability or prosessing Thunderbird.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
I think that using switch statement increases maintainability of this method.
chatHandler.observe() handle calling methods which topics correspond to.
Using switch statement is better rather than if & return on this purpose.
The correspondence relation of methods and topics line up straight if we take switch statement.
So this increases readability and maintainability.

And I don't think this patch land to comm-aurora.
This patch is made with simple replacement, so I'll make easy the backport.
But this patch does not fix a concrete bug for usability or processing Thunderbird.
Sorry, I duplicated the comment.
(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #5)
> I think that using switch statement increases maintainability of this method.
> chatHandler.observe() handle calling methods which topics correspond to.
> Using switch statement is betther rather than if & return on this purpose.
> The correspondence relation of methods and topics line up straight if we
> take switch statement.
> So this increases readability and maintainability.
Florian and I discussed this briefly on IRC and there's a few things I disagree with that increase the readability and maintainability:
1. People tend to forget break statements at the end of long switch statements (which some of these are very long!).
2. Putting scope blocks into a switch statement seems strange and confusing.
3. It greatly increases the indent of the code (thus decreasing readability). There's an extra indent for each case, plus an extra indent for each scope block.

> And I don't think this patch land to comm-aurora.
> This patch is maked with simple replacement, so I'll make easy the backport.
> But this patch does not fix a concrete bug for usability or prosessing
> Thunderbird.
Right, Florian's point is that it will make committing other patches to comm-aurora more difficult if they touch this code (you can't just commit the same patch, you'll need to rewrite it).

Another option that might improve readability (and that I had discussed with Florian when initially reviewing this, I think) is using else if statements instead of returning at the end of each if statement.
(In reply to Patrick Cloke [:clokep] from comment #8)
> Florian and I discussed this briefly on IRC and there's a few things I
> disagree with that increase the readability and maintainability:
> 1. People tend to forget break statements at the end of long switch
> statements (which some of these are very long!).
> 2. Putting scope blocks into a switch statement seems strange and confusing.
> 3. It greatly increases the indent of the code (thus decreasing
> readability). There's an extra indent for each case, plus an extra indent
> for each scope block.
I feel that forgot break statement is found out by reviewing code. This seriousness is low. If case statement is long and increase indents, splitting subroutine methods from them will resolve them. splitting methods will resolves scope block length.

> Another option that might improve readability (and that I had discussed with
> Florian when initially reviewing this, I think) is using else if statements
> instead of returning at the end of each if statement.
Umm... Do not resolved by splitting sub methods and using switch statement?
FWIW, early returns are much in favor in mozilla code in general, i'd say. (== keep it like it is)
"No else after return" is in the mozilla style guidelines even.
(In reply to Magnus Melin from comment #10)
> FWIW, early returns are much in favor in mozilla code in general, i'd say.
> (== keep it like it is)

Yes it is. 
But also there are some cases that codes handling many sub-routine like chatHandler.observe() uses switch statement.
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7105
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesCategoriesStarter.js#94
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/nsLoginManager.js#336
Per reasons given above, this is WONTFIX.

Sorry, I just don't feel it makes the code anymore readable.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.