Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LANG-1644 - Check if number is hexadecimal #727

Closed
wants to merge 1 commit into from
Closed

LANG-1644 - Check if number is hexadecimal #727

wants to merge 1 commit into from

Conversation

arturobernalg
Copy link
Member

@arturobernalg arturobernalg commented Mar 3, 2021

Checks whether the given String is a hex number.

  • NumberUtils.isHexNumber(null)) = false
  • NumberUtils.isHexNumber("")) = false
  • NumberUtils.isHexNumber("0x12345678")) = true
  • NumberUtils.isHexNumber("0x7fffffffffffffff")) = true
  • NumberUtils.isHexNumber("0x7FFFFFFFFFFFFFFF")) = true
  • NumberUtils.isHexNumber("5D0")) = true
  • NumberUtils.isHexNumber("0x")) = false

@garydgregory
Copy link
Member

You should really run a local build before creating the PR as this PR fails the build.

@arturobernalg
Copy link
Member Author

You should really run a local build before creating the PR as this PR fails the build.

Yep, my mistake. Sorry.

return false;
}
}
for (; i < chars.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be a neat use of our Range class?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @garydgregory

don't you think this may be complicated the code? are you thinking in a global solution, implement in isCreatable perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @garydgregory
I've trying use Range and i think it's more clean check the one by one the char. With range should look like:


 final Range<Integer> range = Range.between(0, 15);

        for (; pfxLen < chars.length; pfxLen++) {
            if (!range.contains(Character.getNumericValue(chars[pfxLen]))) {
                return false;
            }
        }

@coveralls
Copy link

coveralls commented Mar 3, 2021

Coverage Status

Coverage increased (+0.005%) to 94.932% when pulling b47bcdd on arturobernalg:feature/LANG-1644 into 950ab13 on apache:master.

final char[] chars = str.toCharArray();
final int length = chars.length;
int i = 1;
if (chars[i] == 'x' || chars[i] == 'X') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If chars is length 1 then this will throw an index out of bounds exception.

@aherbert
Copy link
Contributor

aherbert commented Mar 4, 2021

Is the intention that this should match isCreateable(String) for all valid hex numbers and return true for all hex cases where a Number is returned from createNumber(String)?

public static boolean isCreatable(final String str)
public static Number createNumber(final String str)

If so then you should support the # character (and all the other hex prefixes) and update the javadoc to reflect the intention.

I would then update the test to use all the hex cases from the tests for createNumber and isCreateable to ensure it returns true when those methods succeed.

Currently your test only covers a range of int and long values, no hex variants and does not specify whether BigInteger values are also true.

It will also fail on valid hex numbers such as 0x1L.

@arturobernalg
Copy link
Member Author

BigInteger

Hi @aherbert

Thank you for your comment. I've fix those cases that you mentioned and i've add more test.

Just one think about --> 0x1L. I don't know what to do with that. If you execute the test compareIsNumberWithCreateNumber("0x1L", true); will throw an exception. Same case with

NumberUtils.isParsable("0x1L")

@aherbert
Copy link
Contributor

aherbert commented Mar 4, 2021

Regarding 0x1L the NumberUtils.createNumber and NumberUtils.isCreatable only supports the preferred type suffix on decimal numbers. You cannot specify a preferred L for hex integers. So this new method should match the existing behaviour of those methods. So you can ignore this.

I am not sure that allowing the number 5D0 to be true is valid. This would not be parsed or created as a Hex integer by any method in NumberUtils. Integer.decode("5D0") throws an exception. Thus a hex number should be identified with 0x, 0X or # with an optional sign prefix (see [LANG-1645] and #728) and thereafter only contain the digits [0-9A-Fa-f].

This method then becomes a subset of isCreateable that will return true only if the Number created by createNumber is a mathematical integer.

@arturobernalg
Copy link
Member Author

Regarding 0x1L the NumberUtils.createNumber and NumberUtils.isCreatable only supports the preferred type suffix on decimal numbers. You cannot specify a preferred L for hex integers. So this new method should match the existing behaviour of those methods. So you can ignore this.

I am not sure that allowing the number 5D0 to be true is valid. This would not be parsed or created as a Hex integer by any method in NumberUtils. Integer.decode("5D0") throws an exception. Thus a hex number should be identified with 0x, 0X or # with an optional sign prefix (see [LANG-1645] and #728) and thereafter only contain the digits [0-9A-Fa-f].

This method then becomes a subset of isCreateable that will return true only if the Number created by createNumber is a mathematical integer.

HI @aherbert

Depends on what we want to achieve with the method. 5D0 it's a valid hex and IMO should be considered for the method and returned true.

@aherbert
Copy link
Contributor

aherbert commented Mar 4, 2021

The value 5D0 may be valid hex digits but what are you going to do with that fact? If you want to create a number with it then nothing in NumberUtils or the Java language will make a number from that string value. They all require that the string is marked with a prefix to identify it should be parsed as hex.

For example the following is valid hex 444 but if you parse it as 0x444 and 444 you get different numbers.

So what is the use case for a method that identifies if a string has only valid hexidecimal digits and optionally a minus and a hex prefix?

@arturobernalg
Copy link
Member Author

arturobernalg commented Mar 5, 2021

Hi @aherbert

I will follow your approach.

Regards

@arturobernalg arturobernalg closed this by deleting the head repository Jun 22, 2023
@aherbert aherbert mentioned this pull request Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants