[sip-comm-dev] [Patch] OS detection

Archive
1

Hi,

In SC sources I found a lot of "manually" checks to find OS name (private static isWindows() or inline System.getProperty("os.name").startsWith("Mac"),...).

I propose to have a unique class that tell us OS name and architecture (32/64 bit) called OSUtils in net.java.sip.communicator.util (patch attached is against revision 6459).
I think it will improve code readability and reduce code duplication.

WDYT ?

Best regards,

sipcomm-osutils.diff (30.5 KB)

···

--
Seb

2

Hi Seb,

Thank you for the good idea and contribution! The patch is now committed into trunk as r6462 and acknowledged on our "Team and Contributors" page.

I made modifications such as:
- fixing code to agree with the javadocs
- removing duplications
- keeping the code lines to 80 chars
- adding <tt> and </tt> around keywords in javadocs
- ordering imports

I think we can get a bigger bang for the effort if we optimize the functions in OSUtils or rather replace them with constants. For what it's worth, os.name isn't supposed to change while the application is running and I'm not sure that our code which depends on it can handle a change at runtime. That's why I think we spare ourselves the string comparisons on each and every call to the functions and we could just calculate the values once and then expose them as constants.

Regards,
Lubomir

···

On 9.12.2009 13:59, Sebastien Vincent wrote:

Hi,

In SC sources I found a lot of "manually" checks to find OS name (private static isWindows() or inline System.getProperty("os.name").startsWith("Mac"),...).

I propose to have a unique class that tell us OS name and architecture (32/64 bit) called OSUtils in net.java.sip.communicator.util (patch attached is against revision 6459).
I think it will improve code readability and reduce code duplication.

WDYT ?

Best regards,
--
Seb

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

3

OK I agree, it is better :). I will do that.

···

On 09/12/2009 15:14, Lubomir Marinov wrote:

Hi Seb,

Thank you for the good idea and contribution! The patch is now committed into trunk as r6462 and acknowledged on our "Team and Contributors" page.

I made modifications such as:
- fixing code to agree with the javadocs
- removing duplications
- keeping the code lines to 80 chars
- adding <tt> and </tt> around keywords in javadocs
- ordering imports

I think we can get a bigger bang for the effort if we optimize the functions in OSUtils or rather replace them with constants. For what it's worth, os.name isn't supposed to change while the application is running and I'm not sure that our code which depends on it can handle a change at runtime. That's why I think we spare ourselves the string comparisons on each and every call to the functions and we could just calculate the values once and then expose them as constants.

--
Seb

Regards,
Lubomir

On 9.12.2009 13:59, Sebastien Vincent wrote:

Hi,

In SC sources I found a lot of "manually" checks to find OS name (private static isWindows() or inline System.getProperty("os.name").startsWith("Mac"),...).

I propose to have a unique class that tell us OS name and architecture (32/64 bit) called OSUtils in net.java.sip.communicator.util (patch attached is against revision 6459).
I think it will improve code readability and reduce code duplication.

WDYT ?

Best regards,
--
Seb

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

4

Hi,

Here is patch against revision 6463. OSUtils class now just exposes constants for OS and architecture.

Regards,

osutils-fields.diff (25.6 KB)

···

--
Seb

On 09/12/2009 15:52, Sebastien Vincent wrote:

On 09/12/2009 15:14, Lubomir Marinov wrote:

Hi Seb,

Thank you for the good idea and contribution! The patch is now committed into trunk as r6462 and acknowledged on our "Team and Contributors" page.

I made modifications such as:
- fixing code to agree with the javadocs
- removing duplications
- keeping the code lines to 80 chars
- adding <tt> and </tt> around keywords in javadocs
- ordering imports

I think we can get a bigger bang for the effort if we optimize the functions in OSUtils or rather replace them with constants. For what it's worth, os.name isn't supposed to change while the application is running and I'm not sure that our code which depends on it can handle a change at runtime. That's why I think we spare ourselves the string comparisons on each and every call to the functions and we could just calculate the values once and then expose them as constants.

OK I agree, it is better :). I will do that.

--
Seb

Regards,
Lubomir

On 9.12.2009 13:59, Sebastien Vincent wrote:

Hi,

In SC sources I found a lot of "manually" checks to find OS name (private static isWindows() or inline System.getProperty("os.name").startsWith("Mac"),...).

I propose to have a unique class that tell us OS name and architecture (32/64 bit) called OSUtils in net.java.sip.communicator.util (patch attached is against revision 6459).
I think it will improve code readability and reduce code duplication.

WDYT ?

Best regards,
--
Seb

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

5

Seb,

Thank you for the refactorying! It's now committed into trunk as r6467.

Regards,
Lubomir

···

On 9.12.2009 17:45, Sebastien Vincent wrote:

Hi,

Here is patch against revision 6463. OSUtils class now just exposes constants for OS and architecture.

Regards,
--
Seb

On 09/12/2009 15:52, Sebastien Vincent wrote:

On 09/12/2009 15:14, Lubomir Marinov wrote:

Hi Seb,

Thank you for the good idea and contribution! The patch is now committed into trunk as r6462 and acknowledged on our "Team and Contributors" page.

I made modifications such as:
- fixing code to agree with the javadocs
- removing duplications
- keeping the code lines to 80 chars
- adding <tt> and </tt> around keywords in javadocs
- ordering imports

I think we can get a bigger bang for the effort if we optimize the functions in OSUtils or rather replace them with constants. For what it's worth, os.name isn't supposed to change while the application is running and I'm not sure that our code which depends on it can handle a change at runtime. That's why I think we spare ourselves the string comparisons on each and every call to the functions and we could just calculate the values once and then expose them as constants.

OK I agree, it is better :). I will do that.

--
Seb

Regards,
Lubomir

On 9.12.2009 13:59, Sebastien Vincent wrote:

Hi,

In SC sources I found a lot of "manually" checks to find OS name (private static isWindows() or inline System.getProperty("os.name").startsWith("Mac"),...).

I propose to have a unique class that tell us OS name and architecture (32/64 bit) called OSUtils in net.java.sip.communicator.util (patch attached is against revision 6459).
I think it will improve code readability and reduce code duplication.

WDYT ?

Best regards,
--
Seb

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net