Code Review

[Vineeth Menon]

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?

  /**
   * This function returns boolean value if the XCUITest needs to be used. Throws a
   * IllegalStateException if the length of the @param version is 0.
   *
   * @param version The OS version of the device
   * @return True if XCUITest is needed.
   * @see <a href="http://google.github.io/guava/releases/snapshot/api/docs/com/google/common/base/Preconditions.html#checkState-boolean-" rel="nofollow">Google
   * Guava checkState</a>
   */
  private boolean isXcui(String version) {
    Preconditions.checkState(version.length() < 1);
    int[] baseVersion = {9, 3};
    boolean flag = false;
    for (int i = 0; i < baseVersion.length; i++) {
      flag = osVersionConvertor(version)[i] > baseVersion[i] ? true : false;
    }
    return flag;
  }

  /**
   * This function will convert a number with two decimal places from String to an int array. Throws
   * a Runtime Exception (IllegalStateException) if there are more that 3 decimal places.
   *
   * @param version The version String.
   * @return The array of int with version numbers.
   * @see <a href="https://docs.oracle.com/javase/8/docs/api/index.html?java/lang/IllegalStateException.html" rel="nofollow">Illegal
   * State Exception</a>
   */
  private int[] osVersionConvertor(String version) {
    if (version.matches("\\d(\\d)?.\\d(\\d)?(\\.\\d(\\d)?)?")) {
      String versionSplit[] = version.split("\\.");
      int[] versionIntegers = {};
      for (int i = 0; i < versionSplit.length; i++) {
        versionIntegers = ArrayUtils.add(versionIntegers, Integer.parseInt(versionSplit[i]));
      }
      return versionIntegers;
    } else {
      throw new IllegalStateException(
          "App version should not have more than 2 decimal places. Refer Docs.");
    }
  }

[Campbell Ritchie]

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.

[Liutauras Vilda]

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]

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?

  private boolean isXcui(String version) {
    Preconditions.checkState(version.length() < 1);
    int[] baseVersion = {9, 3};
    int[] actualVersion = osVersionConvertor(version);
    boolean flag = false;
    for (int i = 0; i < baseVersion.length; i++) {
      flag = actualVersion[i] > baseVersion[i];
      if (flag) {
        break;
      }
    }
    return flag;
  }

[Liutauras Vilda]

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]

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?

[Jim Venolia]

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]

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

That's a pleasure

This is how I originally wrote the code. . . .    boolean flag = false;    for (int i = 0; i < baseVersion.length; i++) {      flag = actualVersion[i] > baseVersion[i];      if (flag) {        break;      }    }    return flag;  }

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]

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]

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

  /**
   * This function returns true if the XCUITest needs to be used, else false. Throws a
   * IllegalArgumentException if the length of the @param version is 0.
   *
   * @param version The OS version of the device.
   * @return True if XCUITest is needed.
   * @see <a href="https://docs.oracle.com/javase/8/docs/api/index.html?java/lang/IllegalArgumentException.html" rel="nofollow">Illegal
   * Argument Exception</a>
   */
  private boolean isXcuiTestRequired(String version) {
    if (version.length() == 0) {
      throw new IllegalArgumentException("");
    }
    int[] baseVersion = {9, 3, 0};
    int[] currentVersion = osVersionConvertor(version);
    boolean xcuiRequirement = false;
    for (int i = 0; i < baseVersion.length; i++) {
      xcuiRequirement = (currentVersion[i] > baseVersion[i]) ? true : false;
    }
    return xcuiRequirement;
  }

  /**
   * This function will convert a number with two decimal places from String to an int array. Throws
   * a Runtime Exception (IllegalStateException) if there are more that 2 decimal places.
   *
   * @param version The version String.
   * @return The array of int with version numbers.
   * @see <a href="https://docs.oracle.com/javase/8/docs/api/index.html?java/lang/IllegalArgumentException.html" rel="nofollow">Illegal
   * Argument Exception</a>
   */
  private int[] osVersionConvertor(String version) {
    if (version.matches("\\d(\\d)?.\\d(\\d)?(\\.\\d(\\d)?)?")) {
      String versionSplit[] = version.split("\\.");
      int[] versionIntegers = {};
      for (int i = 0; i < versionSplit.length; i++) {
        versionIntegers = ArrayUtils.add(versionIntegers, Integer.parseInt(versionSplit[i]));
      }
      return versionIntegers;
    } else {
      throw new IllegalArgumentException(
          "App version should not have more than 2 decimal places. Refer xTune Docs.");
    }
  }

Once again, thank a lot for the comments guys... Appreciate all the help.

[Vineeth Menon]

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]

Vineeth Menon wrote:

if (version.length() == 0) {
      throw new IllegalArgumentException("");
}

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 (version.isEmpty()) {
    // throw exception
}

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.

private boolean versionSpecified(String version) {
    String v = Objects.requireNonNull(version, "version is null");
    return !v.trim().isEmpty();
}

Now you have more descriptive method, which reads:

if (!versionSpecified(version)) {
    // throw exception
}

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]

Vineeth Menon wrote:

throw new IllegalArgumentException("App version should not have more than 2 decimal places. Refer xTune Docs.");

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]

for (int i = 0; i < baseVersion.length; i++) {
    xcuiRequirement = (currentVersion[i] > baseVersion[i]) ? true : false;
}
return xcuiRequirement;

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]

By the way, don't take comments from the negative side, we are all up for a help to improve your code

[Vineeth Menon]

Nope, I do get your point. And thanks for pointing out rather than sugarcoating the message.  

[Campbell Ritchie]

Also don't use xxx ? true : false; Use xxx alone.