You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Igal Sapir <is...@apache.org> on 2018/10/02 22:54:04 UTC
Refactoring and Cleanup of OS Name property usage
Rainer pointed out to me the class JrePlatform [1], which has a helper
field called IS_WINDOWS.
I think that it would make sense to add a constant field OS_NAME, as
well as IS_LINUX and IS_MACOS, and use these fields instead of calling
System.getProperty("os.name") in multiple places - some examples [2] [3]
[4].
If there is no objection, I can go ahead and refactor that.
Best,
Igal
[1]
https://github.com/apache/tomcat/blob/trunk/java/org/apache/tomcat/util/compat/JrePlatform.java
[2]
https://github.com/apache/tomcat/blob/trunk/java/org/apache/catalina/util/ServerInfo.java#L113
[3]
https://github.com/apache/tomcat/blob/trunk/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/FairBlockingQueue.java#L56
[4]
https://github.com/apache/tomcat/blob/trunk/test/org/apache/tomcat/util/net/TesterSupport.java#L196
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Refactoring and Cleanup of OS Name property usage
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Igal,
On 10/2/18 18:54, Igal Sapir wrote:
> Rainer pointed out to me the class JrePlatform [1], which has a
> helper field called IS_WINDOWS.
>
> I think that it would make sense to add a constant field OS_NAME,
> as well as IS_LINUX and IS_MACOS, and use these fields instead of
> calling System.getProperty("os.name") in multiple places - some
> examples [2] [3] [4].
>
> If there is no objection, I can go ahead and refactor that.
+1
- -chris
-----BEGIN PGP SIGNATURE-----
Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/
iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlu09ogACgkQHPApP6U8
pFjXHQ//XUz9mDXA8hUFxL+6Ck/xjDiExUjTFwV7rm0hYWjaeECHqz5tgZ5h3FAy
2S2JaICB1+BhzBPQAkVrruadIqy4X5pIOs4cekZcOGtxThxRTPPOTedhjktXMy6s
Yd5p0DHpkqy2zZFUQWnpXfuQNqrQv3fL6Reqed5EOeoFRvTKjGM8XiUeoRA1+rIB
r0fT0zZDO5im9GEdev4nSbrUE74vPZMHnBH1ykKMB9zWQVwnkIfajCaJMuV7glrj
T6dOJa6z15hMfSOkrX0xy6AaB0GpMhh9kZuYjySXW5n6tPNIMNNX+x+j+Xu7nQnS
MRkYe+2zNSS8GUT638e026loH8XDA3ortvnXZ+mdK01RMGe+4hRR2OKPoaok3qaQ
tInLACtWun6tEsAtGLhVcn2gEO60y22r+d+bOLKOz7G7+9D1aKRQIfMnO9wc5ogG
O4dAhlSXLn3fE16vlL1jyzbje3D85jEUYT6tkT7ee307BbVflfZ9+OGb8tUjjfMl
iUNWmUz6ly4caZD28VlenLM8rRu4HZZ/684wQl6SvJCAp60Irk/WfM7T1zNO3vjr
xVaR+S3G+elNLmnoGLcF6538aket7x+1ixnG67Cs3gRRlhtuSMEHThZHNTR4ecob
uPu9eiKfIaa3W7g67FPQkxZp2TnzeshPKUjTMdoiX0V4N3Zz2kU=
=J0yo
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Refactoring and Cleanup of OS Name property usage
Posted by Igal Sapir <is...@apache.org>.
On 10/3/2018 12:05 PM, Mark Thomas wrote:
>>>>> [4]
>>>>> https://github.com/apache/tomcat/blob/trunk/test/org/apache/tomcat/util/net/TesterSupport.java#L196
>>>>>
>>> That one I do think makes sense to pull into JrePlatform.
>> I have moved [4] above in r1842748. I used the existing convention in
>> the file of setting the string to a static variable, i.e.
>>
>> private static final String OS_NAME_WINDOWS_PREFIX = "Windows";
>> + private static final String OS_NAME_MAC_OS_LC_PREFIX = "mac os x";
>>
>>
>> though I personally think that it would make sense to inline it since
>> it's only used once.
> It would also mean fewer lines of code - always a winner in my book ;)
'nuff said - r1842751
> To be honest, I have no strong feelings one way or the other. It is a
> style choice more than anything else in this case. It isn't an itch I
> want to scratch but equally I'm not going to object if you want to make
> that change.
To itch his own ;)
Igal
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Refactoring and Cleanup of OS Name property usage
Posted by Mark Thomas <ma...@apache.org>.
On 03/10/18 19:23, Igal Sapir wrote:
> On 10/3/2018 4:41 AM, Mark Thomas wrote:
<snip/>
>>>> [4]
>>>> https://github.com/apache/tomcat/blob/trunk/test/org/apache/tomcat/util/net/TesterSupport.java#L196
>>>>
>> That one I do think makes sense to pull into JrePlatform.
>
> I have moved [4] above in r1842748. I used the existing convention in
> the file of setting the string to a static variable, i.e.
>
> private static final String OS_NAME_WINDOWS_PREFIX = "Windows";
> + private static final String OS_NAME_MAC_OS_LC_PREFIX = "mac os x";
>
>
> though I personally think that it would make sense to inline it since
> it's only used once.
It would also mean fewer lines of code - always a winner in my book ;)
To be honest, I have no strong feelings one way or the other. It is a
style choice more than anything else in this case. It isn't an itch I
want to scratch but equally I'm not going to object if you want to make
that change.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Refactoring and Cleanup of OS Name property usage
Posted by Igal Sapir <is...@apache.org>.
On 10/3/2018 4:41 AM, Mark Thomas wrote:
> On 03/10/18 00:48, Igal Sapir wrote:
>> Regarding this:
>>
>> On 10/2/2018 3:54 PM, Igal Sapir wrote:
>>> Rainer pointed out to me the class JrePlatform [1], which has a helper
>>> field called IS_WINDOWS.
>>>
>>> I think that it would make sense to add a constant field OS_NAME, as
>>> well as IS_LINUX and IS_MACOS, and use these fields instead of calling
>>> System.getProperty("os.name") in multiple places - some examples [2]
>>> [3] [4].
>>>
>>> If there is no objection, I can go ahead and refactor that.
> Hmm. We do have a lot of calls to System.getProperty("os.name") but they
> nearly all appear to be used alongside other calls to
> System.getProperty(...) and are not further tested for specific OS values.
>
> I wonder if, from a maintenance point of view, it wouldn't be clearer to
> leave those as they are? I guess I am -0 on that part of the refactoring.
Fair enough.
> <snip/>
>
>>> [4]
>>> https://github.com/apache/tomcat/blob/trunk/test/org/apache/tomcat/util/net/TesterSupport.java#L196
> That one I do think makes sense to pull into JrePlatform.
I have moved [4] above in r1842748. I used the existing convention in
the file of setting the string to a static variable, i.e.
private static final String OS_NAME_WINDOWS_PREFIX = "Windows";
+ private static final String OS_NAME_MAC_OS_LC_PREFIX = "mac os x";
though I personally think that it would make sense to inline it since
it's only used once.
Igal
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Refactoring and Cleanup of OS Name property usage
Posted by Mark Thomas <ma...@apache.org>.
On 03/10/18 00:48, Igal Sapir wrote:
> Regarding this:
>
> On 10/2/2018 3:54 PM, Igal Sapir wrote:
>> Rainer pointed out to me the class JrePlatform [1], which has a helper
>> field called IS_WINDOWS.
>>
>> I think that it would make sense to add a constant field OS_NAME, as
>> well as IS_LINUX and IS_MACOS, and use these fields instead of calling
>> System.getProperty("os.name") in multiple places - some examples [2]
>> [3] [4].
>>
>> If there is no objection, I can go ahead and refactor that.
Hmm. We do have a lot of calls to System.getProperty("os.name") but they
nearly all appear to be used alongside other calls to
System.getProperty(...) and are not further tested for specific OS values.
I wonder if, from a maintenance point of view, it wouldn't be clearer to
leave those as they are? I guess I am -0 on that part of the refactoring.
>> [1]
>> https://github.com/apache/tomcat/blob/trunk/java/org/apache/tomcat/util/compat/JrePlatform.java
>>
>>
>> [2]
>> https://github.com/apache/tomcat/blob/trunk/java/org/apache/catalina/util/ServerInfo.java#L113
>>
>>
>> [3]
>> https://github.com/apache/tomcat/blob/trunk/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/FairBlockingQueue.java#L56
>>
>>
>
> Example [3] above is in a separate module, so perhaps those should not
> be dependent on org.apache.tomcat? I'm not sure but wanted to point it
> out.
The jdbc-pool module only has a dependency on JULI - tomcat's logging
framework - which itself has no other dependencies. I don't think the
benefits of this change justify adding tomcat-util.jar as an additional
dependency.
>> [4]
>> https://github.com/apache/tomcat/blob/trunk/test/org/apache/tomcat/util/net/TesterSupport.java#L196
That one I do think makes sense to pull into JrePlatform.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Refactoring and Cleanup of OS Name property usage
Posted by Igal Sapir <is...@apache.org>.
Regarding this:
On 10/2/2018 3:54 PM, Igal Sapir wrote:
> Rainer pointed out to me the class JrePlatform [1], which has a helper
> field called IS_WINDOWS.
>
> I think that it would make sense to add a constant field OS_NAME, as
> well as IS_LINUX and IS_MACOS, and use these fields instead of calling
> System.getProperty("os.name") in multiple places - some examples [2]
> [3] [4].
>
> If there is no objection, I can go ahead and refactor that.
>
> [1]
> https://github.com/apache/tomcat/blob/trunk/java/org/apache/tomcat/util/compat/JrePlatform.java
>
> [2]
> https://github.com/apache/tomcat/blob/trunk/java/org/apache/catalina/util/ServerInfo.java#L113
>
> [3]
> https://github.com/apache/tomcat/blob/trunk/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/FairBlockingQueue.java#L56
>
Example [3] above is in a separate module, so perhaps those should not
be dependent on org.apache.tomcat? I'm not sure but wanted to point it out.
Igal
> [4]
> https://github.com/apache/tomcat/blob/trunk/test/org/apache/tomcat/util/net/TesterSupport.java#L196
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org