741072 - Use switch statement in chatHandler.observe()

Status: RESOLVED WONTFIX

(Thunderbird :: Instant Messaging, defect)

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Use switch statement for clean up.

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: proposed patch

Comment 2

[Patrick Cloke [:clokep]]

(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. :)

Comment 3

[Florian Quèze [: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.

Comment 4

[Florian Quèze [:florian]]

Resolving to WONTFIX (feel free to reopen if you see a really good reason for us to want this change :)).

Comment 5

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

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.

Comment 6

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

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.

Comment 7

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Sorry, I duplicated the comment.

Comment 8

[Patrick Cloke [:clokep]]

(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.

Comment 9

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(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?

Comment 10

[Magnus Melin [:mkmelin]]

FWIW, early returns are much in favor in mozilla code in general, i'd say. (== keep it like it is)

Comment 11

[Magnus Melin [:mkmelin]]

"No else after return" is in the mozilla style guidelines even.

Comment 12

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(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

Comment 13

[Patrick Cloke [:clokep]]

Per reasons given above, this is WONTFIX.

Sorry, I just don't feel it makes the code anymore readable.