You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@karaf.apache.org by Glen Mazza <gl...@gmail.com> on 2011/09/23 14:07:09 UTC
Hmm. (Re: commit: r1174535)
Added:
karaf/trunk/management/mbeans/http/src/main/java/org/apache/karaf/management/mbeans/http/internal/HttpMBeanImpl.java
> URL: http://svn.apache.org/viewvc/karaf/trunk/management/mbeans/http/src/main/java/org/apache/karaf/management/mbeans/http/internal/HttpMBeanImpl.java?rev=1174535&view=auto
> ==============================================================================
> --- karaf/trunk/management/mbeans/http/src/main/java/org/apache/karaf/management/mbeans/http/internal/HttpMBeanImpl.java (added)
> +++ karaf/trunk/management/mbeans/http/src/main/java/org/apache/karaf/management/mbeans/http/internal/HttpMBeanImpl.java Fri Sep 23 06:07:32 2011
> @@ -0,0 +1,101 @@
> +/*
> + private String getStateString(int type) {
> + switch(type) {
> + case WebEvent.DEPLOYING:
> + return "Deploying ";
> + case WebEvent.DEPLOYED:
> + return "Deployed ";
> + case WebEvent.UNDEPLOYING:
> + return "Undeploying";
> + case WebEvent.UNDEPLOYED:
> + return "Undeployed ";
> + case WebEvent.FAILED:
> + return "Failed ";
> + case WebEvent.WAITING:
> + return "Waiting ";
> + default:
> + return "Failed ";
> + }
JB, are you sure you want to have the same "default" value as you have
for another case (WebEvent.FAILED)? Might you want to make it a
different value ("Unknown", for example, or "Invalid" or "Error" if the
"default" case should never be occurring?)
Reason: if someone on the mailing list complains that the state is
"Failed" and they want to know why, it's harder to trace the code
because you don't know if the case is WebEvent.FAILED or default
(something else), because they both give the same text string. Whereas
if the values are different you'll know if the WebEvent actually did
fail, or somewhere else in the code, the type value wasn't being
properly set and hence the code was improperly falling into the
"default" category.
Glen
--
Glen Mazza
Talend - http://www.talend.com/ai
Blog - http://www.jroller.com/gmazza
Twitter - glenmazza
Re: Hmm. (Re: commit: r1174535)
Posted by Achim Nierbeck <bc...@googlemail.com>.
Hi Glen,
yeah a unknow could be better here, though should never happen and if
you see that one you're system is down the drain :-)
Regards, Achim
2011/9/23 Glen Mazza <gl...@gmail.com>:
> Added:
> karaf/trunk/management/mbeans/http/src/main/java/org/apache/karaf/management/mbeans/http/internal/HttpMBeanImpl.java
>>
>> URL:
>> http://svn.apache.org/viewvc/karaf/trunk/management/mbeans/http/src/main/java/org/apache/karaf/management/mbeans/http/internal/HttpMBeanImpl.java?rev=1174535&view=auto
>>
>> ==============================================================================
>> ---
>> karaf/trunk/management/mbeans/http/src/main/java/org/apache/karaf/management/mbeans/http/internal/HttpMBeanImpl.java
>> (added)
>> +++
>> karaf/trunk/management/mbeans/http/src/main/java/org/apache/karaf/management/mbeans/http/internal/HttpMBeanImpl.java
>> Fri Sep 23 06:07:32 2011
>> @@ -0,0 +1,101 @@
>> +/*
>> + private String getStateString(int type) {
>> + switch(type) {
>> + case WebEvent.DEPLOYING:
>> + return "Deploying ";
>> + case WebEvent.DEPLOYED:
>> + return "Deployed ";
>> + case WebEvent.UNDEPLOYING:
>> + return "Undeploying";
>> + case WebEvent.UNDEPLOYED:
>> + return "Undeployed ";
>> + case WebEvent.FAILED:
>> + return "Failed ";
>> + case WebEvent.WAITING:
>> + return "Waiting ";
>> + default:
>> + return "Failed ";
>> + }
>
> JB, are you sure you want to have the same "default" value as you have for
> another case (WebEvent.FAILED)? Might you want to make it a different value
> ("Unknown", for example, or "Invalid" or "Error" if the "default" case
> should never be occurring?)
>
> Reason: if someone on the mailing list complains that the state is "Failed"
> and they want to know why, it's harder to trace the code because you don't
> know if the case is WebEvent.FAILED or default (something else), because
> they both give the same text string. Whereas if the values are different
> you'll know if the WebEvent actually did fail, or somewhere else in the
> code, the type value wasn't being properly set and hence the code was
> improperly falling into the "default" category.
>
> Glen
>
> --
> Glen Mazza
> Talend - http://www.talend.com/ai
> Blog - http://www.jroller.com/gmazza
> Twitter - glenmazza
>
>
--
--
*Achim Nierbeck*
Apache Karaf <http://karaf.apache.org/> Committer & PMC
OPS4J Pax Web <http://wiki.ops4j.org/display/paxweb/Pax+Web/>
Committer & Project Lead
blog <http://notizblog.nierbeck.de/>
Re: Hmm. (Re: commit: r1174535)
Posted by Achim Nierbeck <bc...@googlemail.com>.
Hi JB,
yeah, if your at it fix both ;)
Regards, Achim
2011/9/23 Jean-Baptiste Onofré <jb...@nanthrax.net>:
> Hi Glen,
>
> I applied the same behavior as used in the http:* commands ;)
> We can improve both.
>
> Regards
> JB
>
> On 09/23/2011 02:07 PM, Glen Mazza wrote:
>>
>> Added:
>>
>> karaf/trunk/management/mbeans/http/src/main/java/org/apache/karaf/management/mbeans/http/internal/HttpMBeanImpl.java
>>
>>> URL:
>>>
>>> http://svn.apache.org/viewvc/karaf/trunk/management/mbeans/http/src/main/java/org/apache/karaf/management/mbeans/http/internal/HttpMBeanImpl.java?rev=1174535&view=auto
>>>
>>>
>>> ==============================================================================
>>>
>>> ---
>>>
>>> karaf/trunk/management/mbeans/http/src/main/java/org/apache/karaf/management/mbeans/http/internal/HttpMBeanImpl.java
>>> (added)
>>> +++
>>>
>>> karaf/trunk/management/mbeans/http/src/main/java/org/apache/karaf/management/mbeans/http/internal/HttpMBeanImpl.java
>>> Fri Sep 23 06:07:32 2011
>>> @@ -0,0 +1,101 @@
>>> +/*
>>> + private String getStateString(int type) {
>>> + switch(type) {
>>> + case WebEvent.DEPLOYING:
>>> + return "Deploying ";
>>> + case WebEvent.DEPLOYED:
>>> + return "Deployed ";
>>> + case WebEvent.UNDEPLOYING:
>>> + return "Undeploying";
>>> + case WebEvent.UNDEPLOYED:
>>> + return "Undeployed ";
>>> + case WebEvent.FAILED:
>>> + return "Failed ";
>>> + case WebEvent.WAITING:
>>> + return "Waiting ";
>>> + default:
>>> + return "Failed ";
>>> + }
>>
>> JB, are you sure you want to have the same "default" value as you have
>> for another case (WebEvent.FAILED)? Might you want to make it a
>> different value ("Unknown", for example, or "Invalid" or "Error" if the
>> "default" case should never be occurring?)
>>
>> Reason: if someone on the mailing list complains that the state is
>> "Failed" and they want to know why, it's harder to trace the code
>> because you don't know if the case is WebEvent.FAILED or default
>> (something else), because they both give the same text string. Whereas
>> if the values are different you'll know if the WebEvent actually did
>> fail, or somewhere else in the code, the type value wasn't being
>> properly set and hence the code was improperly falling into the
>> "default" category.
>>
>> Glen
>>
>
> --
> Jean-Baptiste Onofré
> jbonofre@apache.org
> http://blog.nanthrax.net
> Talend - http://www.talend.com
>
--
--
*Achim Nierbeck*
Apache Karaf <http://karaf.apache.org/> Committer & PMC
OPS4J Pax Web <http://wiki.ops4j.org/display/paxweb/Pax+Web/>
Committer & Project Lead
blog <http://notizblog.nierbeck.de/>
Re: Hmm. (Re: commit: r1174535)
Posted by Jean-Baptiste Onofré <jb...@nanthrax.net>.
Hi Glen,
I applied the same behavior as used in the http:* commands ;)
We can improve both.
Regards
JB
On 09/23/2011 02:07 PM, Glen Mazza wrote:
> Added:
> karaf/trunk/management/mbeans/http/src/main/java/org/apache/karaf/management/mbeans/http/internal/HttpMBeanImpl.java
>
>> URL:
>> http://svn.apache.org/viewvc/karaf/trunk/management/mbeans/http/src/main/java/org/apache/karaf/management/mbeans/http/internal/HttpMBeanImpl.java?rev=1174535&view=auto
>>
>> ==============================================================================
>>
>> ---
>> karaf/trunk/management/mbeans/http/src/main/java/org/apache/karaf/management/mbeans/http/internal/HttpMBeanImpl.java
>> (added)
>> +++
>> karaf/trunk/management/mbeans/http/src/main/java/org/apache/karaf/management/mbeans/http/internal/HttpMBeanImpl.java
>> Fri Sep 23 06:07:32 2011
>> @@ -0,0 +1,101 @@
>> +/*
>> + private String getStateString(int type) {
>> + switch(type) {
>> + case WebEvent.DEPLOYING:
>> + return "Deploying ";
>> + case WebEvent.DEPLOYED:
>> + return "Deployed ";
>> + case WebEvent.UNDEPLOYING:
>> + return "Undeploying";
>> + case WebEvent.UNDEPLOYED:
>> + return "Undeployed ";
>> + case WebEvent.FAILED:
>> + return "Failed ";
>> + case WebEvent.WAITING:
>> + return "Waiting ";
>> + default:
>> + return "Failed ";
>> + }
>
> JB, are you sure you want to have the same "default" value as you have
> for another case (WebEvent.FAILED)? Might you want to make it a
> different value ("Unknown", for example, or "Invalid" or "Error" if the
> "default" case should never be occurring?)
>
> Reason: if someone on the mailing list complains that the state is
> "Failed" and they want to know why, it's harder to trace the code
> because you don't know if the case is WebEvent.FAILED or default
> (something else), because they both give the same text string. Whereas
> if the values are different you'll know if the WebEvent actually did
> fail, or somewhere else in the code, the type value wasn't being
> properly set and hence the code was improperly falling into the
> "default" category.
>
> Glen
>
--
Jean-Baptiste Onofré
jbonofre@apache.org
http://blog.nanthrax.net
Talend - http://www.talend.com