Help coderanch get a
new server
by contributing to the fundraiser
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Jeanne Boyarsky
  • Ron McLeod
  • Paul Clapham
  • Liutauras Vilda
Sheriffs:
  • paul wheaton
  • Rob Spoor
  • Devaka Cooray
Saloon Keepers:
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
  • Frits Walraven
  • Tim Moores
Bartenders:
  • Mikalai Zaikin

Code Review

 
Ranch Hand
Posts: 91
IntelliJ IDE Tomcat Server Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hi,

In the below piece of code Line #15 I am calling the osVersionConvertor function rather than firstly assigning the value of that to a variable. My intention was to reduce memory footprint as much as possible. Is this the right way?

 
Marshal
Posts: 79531
379
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Vineeth Menon wrote:. . . My intention was to reduce memory footprint as much as possible. . . .

I can't see whether you will really reduce your memory footprint, but why are you worrying about a few hundred bytes? You would have worried about such memory consumption in 1972, but not today.

Line 15 looks to me to be poor style and almost certainly incorrect semantics. I can see other style problems, but please start by explaining how the validation of types is supposed to work.
 
Marshal
Posts: 8884
638
Mac OS X VI Editor BSD Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I would suggest to reduce confusion and complexity of this line at first.
Then you would realise it does clearly something not what you want.

And don’t worry about the memory until it is a problem. Often you’d find out it isnt.
 
Vineeth Menon
Ranch Hand
Posts: 91
IntelliJ IDE Tomcat Server Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Thanks to both of the Marshals for the quick reply   . This is how I originally wrote the code. if the actualVersion is greater than the base version then return true, else false. Guess I got carried away. Marshal Campbell, you did also mention that there are other style issues as well, can you please point that out as well?

 
Liutauras Vilda
Marshal
Posts: 8884
638
Mac OS X VI Editor BSD Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
1. Method name isXcui() is poor.
1.1 You wrote javadoc for it, and javadoc has better explanation which could be reflected in method name. i.e.: mustBeXcuiTested() or something, what explains right away what is all about.
1.2 If method throws an exception, there is a notation in javadoc for that @throws.

2.

Preconditions.checkState(version.length() < 1);


This line is poor in two ways:
2.1 method name checkState doesn't give a sense to a user what happens after checking state.
2.2 You are checking if version length is less than 1. Technically length shorter than 1 can be only 0.  So why you don't check version.length == 0 or version.isEmpty() ?

3. Why the base version is hardcoded in the method? What would happen when base version would change? You'd create new isXcui() variant? Or you would change the method?

4. Why the singular version is an array of numbers?

5. In the way you assess version numbers, much easier following the same logic would be to parse to double and compare which number is higher. I'm not saying I suggest that, but that would be simpler than what you have, that would be 1 line instead of these two methods.

 
Liutauras Vilda
Marshal
Posts: 8884
638
Mac OS X VI Editor BSD Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Forgot that one, your javadoc says:

This function returns boolean value if the XCUITest needs to be used.


My argument is, that it returns boolean value in either case. You mean true or false maybe? Which one in particular?
 
Ranch Hand
Posts: 574
VI Editor Chrome Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:but why are you worrying about a few hundred bytes?



Because like me, he learned via Z80 assembler on a TRS-80 where you had 64k total memory, 16k used by the display.  Old habits die hard, I still spend more time than I should trying to save a few bytes here and there.

It's not lost on me my code now runs on a laptop with 6 gigs of memory backed by a petabyte of swap space and I want to store my small numbers in 8 bits instead of 32.......
 
Campbell Ritchie
Marshal
Posts: 79531
379
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Vineeth Menon wrote:Thanks to both of the Marshals . . . .

That's a pleasure

This is how I originally wrote the code. . . .

In which case, I think you have introduced an error when you changed it. That older code might work correctly if version 12.0 counts as a greater version number than version 11.9999999999999999999. No, on thinking about it a bit more, I decided the older code will give incorrect results, too. The newer code makes me suspect you have been guessing about what you should write rather than thinking logically about it. You can guess 1,000,000× and there is  good chance that one of the guesses will be correct. You simply have to find it. Or you can think logically about what you are doing and get it right first time. Before you do that, write down the specification for version numbers which you are seeking. Also what happens if you pass version 9.3?
More style problems:-
  • 1: The name flag for a boolean variable should have gone out with JV's Z80. Give it a descriptive name.
  • 2: You are using the wrong kind of exception; I think the correct type would be IllegalArgumentException. Not certain.
  •  
    Liutauras Vilda
    Marshal
    Posts: 8884
    638
    Mac OS X VI Editor BSD Java
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Liutauras Vilda wrote:2.1 method name checkState doesn't give a sense to a user what happens after checking state.


    So it is Google Guava library code. However, I am still thinking the same. What a weird method name. Method requires “true”, otherwise exception is thrown, while the method says checkState. If you ask me to check temperature, most likely I would tell you what the temperature is.

    Why not checkStateIsTrue, better probably ensureStateIsTrue, verifyStateIsTrue, requireStateTrue, why “state” at all, requireTrue, ensureIsTrue.
     
    Vineeth Menon
    Ranch Hand
    Posts: 91
    IntelliJ IDE Tomcat Server Java
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Wow that's a lot of comments...  this community is loads better than StackOverflow... Love the place..  
    @Liutauras Thanks for your comments. Here are my 2 cents


    1. Method name isXcui() is poor.
    1.1 You wrote javadoc for it, and javadoc has better explanation which could be reflected in method name. i.e.: mustBeXcuiTested() or something, what explains right away what is all about.
    1.2 If method throws an exception, there is a notation in javadoc for that @throws.


    1. I have change the method name. (I think it's a bit better now)
    1.2 Since it's a RuntimeException and since the function is not throwing it (using throws) I did not use the @throws in javaDoc. However, I did specify in the description that the function will throw a runtimeException. Is this a right thing to do?


    This line is poor in two ways:
    2.1 method name checkState doesn't give a sense to a user what happens after checking state.
    2.2 You are checking if version length is less than 1. Technically length shorter than 1 can be only 0.  So why you don't check version.length == 0 or version.isEmpty() ?



    Yeah, Preconditions does throw people into unwanted confusion (especially if they are not familiar with Guava). I have replaced it with a normal if condition and now checking version.length()==0


    3. Why the base version is hardcoded in the method? What would happen when base version would change? You'd create new isXcui() variant? Or you would change the method?


    It's not going to change. Apple set the rules for XCUITest, not me.  


    Because like me, he learned via Z80 assembler on a TRS-80 where you had 64k total memory, 16k used by the display.


    @Jim, I so wish I had the opportunity. But I started coding on a Pentium 3. However I did have a pretty good teacher who worked on assembler and he did leave a lasting impression on me.


    Old habits die hard, I still spend more time than I should trying to save a few bytes here and there. It's not lost on me my code now runs on a laptop with 6 gigs of memory backed by a petabyte of swap space and I want to store my small numbers in 8 bits instead of 32.......


    Amen to that brother.... can't agree more. I can relate to that so very much...


    That older code might work correctly if version 12.0 counts as a greater version number than version 11.9999999999999999999


    @Campbell Anything above 11.99 will throw a IllegalStateException, I think you missed the osVersionConvertor function. There is a regex check there.

    Also what happens if you pass version 9.3?

    If I pass 9.3 it will return false. The Business Logic also dictates that anything below 9.3 and 9.3 should not use XCUITest.

    As per your suggestion, I have changed the boolean variable flag to something a bit more meaningful.
    Yes, you are right. Illegal Argument is a better one than Illegal State.  

    There was a small change as well. The base version of the code should be 9.3.0 not 9.3, was a miss from my side.    Here is my updated code...please let me know the comments



    Once again, thank a lot for the comments guys... Appreciate all the help.
     
    Vineeth Menon
    Ranch Hand
    Posts: 91
    IntelliJ IDE Tomcat Server Java
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Liutauras Vilda wrote:
    So it is Google Guava library code. However, I am still thinking the same. What a weird method name. Method requires “true”, otherwise exception is thrown, while the method says checkState. If you ask me to check temperature, most likely I would tell you what the temperature is.

    Why not checkStateIsTrue, better probably ensureStateIsTrue, verifyStateIsTrue, requireStateTrue, why “state” at all, requireTrue, ensureIsTrue.



    I know...It's seriously weird. I need to pay more attention to the docs. The crazy part is that it needs "true", and my function was failing since "version.length() < 1" was not true so I had to do a !(version.length() < 1).

    I opted for a normal if condition... Simple life...
     
    Liutauras Vilda
    Marshal
    Posts: 8884
    638
    Mac OS X VI Editor BSD Java
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Vineeth Menon wrote:


    This part still can be improved. For the version to check length reads a bit weird, semantics somewhat wacky. To check if version is empty would be much better. i.e.

    If to go one more step further, probably can be further refactored and wrapped up to a method which would clearly describe what its concern is.

    Now you have more descriptive method, which reads:

    And in the method above I'd say supposed to be a regex (the one you have in osVersionConverter), to check whether version number comply with syntax rules. Which in turn would require even better method name, so the next call is yours.
     
    Liutauras Vilda
    Marshal
    Posts: 8884
    638
    Mac OS X VI Editor BSD Java
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Vineeth Menon wrote:


    Then you have such exception. Exception is good, but the message it contains look suspicious to me. Let's assume version is "v0.1.3.4", this might is not common but valid version identifier. Now, the message is talking about decimal places. Are we really taking about the fractional numbers when we talk about the versions? Decimal places to me are very related with floating point numbers, while the version identifiers I'm not sure they are.

    It is just to think for you, you might come up with a better idea and might will find the way to improve the message.
     
    Liutauras Vilda
    Marshal
    Posts: 8884
    638
    Mac OS X VI Editor BSD Java
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator


    This part. xcuiRequirement gets assigned new value everytime loop iterates, but you take into account only the last version numbers check result. Meaning if there were some inconsistencies in the middle version number these would get ignored, and if just the last version number would comply with your business rules, method would return incorrect result.

    I'm afraid Campbell was right, you seem to be guessing. That's usually not a good approach.

    Have you ever tried writting tests? Let's say using JUnit library or simple print statements? I'd really recomment to start with these, or at least write these now by specifying many different variation of versions which require further testing and which not and then try whether your method give correct or incorrect results.

    To do code review I think is best on the code which produces correct result. I'm not sure that's the case here.
     
    Liutauras Vilda
    Marshal
    Posts: 8884
    638
    Mac OS X VI Editor BSD Java
    • Likes 1
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    By the way, don't take comments from the negative side, we are all up for a help to improve your code
     
    Vineeth Menon
    Ranch Hand
    Posts: 91
    IntelliJ IDE Tomcat Server Java
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Nope, I do get your point. And thanks for pointing out rather than sugarcoating the message.  
     
    Campbell Ritchie
    Marshal
    Posts: 79531
    379
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Also don't use xxx ? true : false; Use xxx alone.
     
    Something must be done about this. Let's start by reading this tiny ad:
    We need your help - Coderanch server fundraiser
    https://coderanch.com/t/782867/Coderanch-server-fundraiser
    reply
      Bookmark Topic Watch Topic
    • New Topic