Validating a number



  • Someone was asked to do validation on a numeric field supplied to us by another system.

    It turns out that they asked to create a db table with all the valid values. Since these were
    the sorts of things that would never change in our industry, they were told to just
    hard code the values. There were way too many for an if-else chain or a switch statement, so
    this individual came up with this:

     

        public enum ValidNumbers {
    _0001,
    _0002,
    ..., // OP: assorted gaps in the range
    _0999,
    1000;

    public static boolean priceIsValid(int n) {
    try {
    ValidNumbers vn = valueOf("
    "+new DecimalFormat("0000").format(n));
    return true;
    } catch (Exception e) {
    return false;
    }
    }
    }


    Maybe someday someone could design an object where you could hash into a map...


  • Considered Harmful

    @snoofle said:

    priceIsValid

    I remember that show.



  • Am I wrong in thinking that this isn't actually that bad? If the numbers mean something, then they're not numbers, they're just unlabelled enumeration values, and this is a perfectly reasonable way to map between them.

    My only nitpick is using an Exception to handle a non-exceptional situation, but that's Java's fault for not having something like "tryParseValue"



  • @pkmnfrk said:

    Am I wrong in thinking that this isn't actually that bad? If the numbers mean something, then they're not numbers, they're just unlabelled enumeration values, and this is a perfectly reasonable way to map between them.

    My only nitpick is using an Exception to handle a non-exceptional situation, but that's Java's fault for not having something like "tryParseValue"

    No, it's bad.  Just use Hashset<int> (or equivalent)

    if(validNumbers.Contains(myInt))...

    On a side note, I JUST came upon this code in an internal library I'm removing from our codebase:

        <font color="#0000ff">public</font> <font color="#0000ff">static</font> Decimal GetDecimal(<font color="#0000ff">object</font> value, Decimal defaultValue = 0M)

    {

    Decimal result;

    <font color="#0000ff">if</font> (value == <font color="#0000ff">null</font> || !Decimal.TryParse(value.ToString(), <font color="#0000ff">out</font> result))

    <font color="#0000ff">return</font> defaultValue;

    <font color="#0000ff">else</font>

    <font color="#0000ff"> </font>

    <font color="#0000ff">return</font> result;

    }

    What do we pass in? A double.



  • At a high level:  If the field is truly numeric, then the leading zeros can be dropped.  On the other hand, if the leading zeros are important, then these things are just strings.  Yes?  Either way, that random-ass underscore is a huge pain. 

    catching(Exception) to handle normal control flow is like a big piece of WTF carrot-cake,

    but the other small things (like incurring the costs of appending the underscore and creating a DecimalFormat each time) are like the cream-cheese icing.  

     




  • Create a bit mask that contains all valid numbers OR'd together. Goes something like 0001 | 0002 ...

    then it's just return (n & VALID_NUMBERS) == n


  • Considered Harmful

    Maybe if the only valid numbers were powers of 2.



  • @joe.edwards said:

    @snoofle said:
    priceIsValid

    I remember that show.

    Doesn't it come on just before root:wheel of /bin/fortune?



  • @joe.edwards said:

    Maybe if the only valid numbers were powers of 2.

    Yeah... well... powers of two are the only good numbers, anyway!



  • @_leonardo_ said:

    At a high level:  If the field is truly numeric, then the leading zeros can be dropped.  On the other hand, if the leading zeros are important, then these things are just strings.  Yes?  Either way, that random-ass underscore is a huge pain. 
    In what way?  The _0001 is the name of an enumeration value.  You can't access it by _1, which is why they have to use 4-digit numbers.  And you can't declare a variable/value that starts with a number, which is why the underscore is there.


  • Considered Harmful

    @Evilweasil said:

    @joe.edwards said:

    Maybe if the only valid numbers were powers of 2.

    Yeah... well... powers of two are the only good numbers, anyway!

    validNumbersMask |= ( 1 << 1 );
    validNumbersMask |= ( 1 << 2 );
    validNumbersMask |= ( 1 << 3 );
    validNumbersMask |= ( 1 << 4 );
    // 5 and 6 are not valid numbers
    validNumbersMask |= ( 1 << 7 );
    validNumbersMask |= ( 1 << 8 );
    validNumbersMask |= ( 1 << 9 );
    ...
    

    Of course, to save time/space you would compute the value then replace it with:

    validNumbersMask = BigInteger.valueOf( "548690845690805689054898789769302576894758967340956987439865734068797472760940968056843258237589732895728957982758972983572687439873984678923689478937987234896789367639878071706987432807602976036326873209476732094679862374...");


  • Considered Harmful

    @db2 said:

    @joe.edwards said:
    @snoofle said:
    priceIsValid

    I remember that show.

    Doesn't it come on just before root:wheel of /bin/fortune?

    No, that's (deal||!deal)



  • I'm glad we had this chat. As time progresses, i'm realizing what a horrible idea this was.



  • @snoofle said:

    Since these were the sorts of things that would never change in our industry

    The worst (and most common) lie told in our industry.


    [mod - to prove a point - PJH]

  • Discourse touched me in a no-no place

    @snoofle said:

    It turns out that they asked to create a db table with all the valid values.
    Since these were the sorts of things that would never change in our
    industry, they were told to just hard code the values.
    Was that the only reason not to use another table? Or was another reason simply premature optimisation? (i.e. saving a lookup/join on a table that happens infrequently enough to not matter at all in the grand scheme of things.)



  • @db2 said:

    @joe.edwards said:
    @snoofle said:
    priceIsValid

    I remember that show.

    Doesn't it come on just before root:wheel of /bin/fortune?

    The show you're probably thinking of is Win, Lose or FILE_NOT_FOUND.

     



  • @PJH said:

    @snoofle said:
    It turns out that they asked to create a db table with all the valid values. Since these were the sorts of things that would never change in our industry, they were told to just hard code the values.
    Was that the only reason not to use another table? Or was another reason simply premature optimisation? (i.e. saving a lookup/join on a table that happens infrequently enough to not matter at all in the grand scheme of things.)
    These constants are the underpinnings of our industry. They've been around for about 40 years, and aren't going to change in any way, short of the American financial system completely collapsing and going away.

    There is a constants file with lots of:

     

      public static int XXX = 1;
    public static int YYY = 2;
    ...

    They put the leading underscore in because you can't start an enum name with a digit. They made them all 4 digits so as to be able to work with a consistent sized number (not necessary as they could have simply done: valueOf("_" + n).

    Of course, putting the constants (mentioned above) into a HashSet would have been the efficient way to go.


  • BINNED

    @snoofle said:

                 ValidNumbers vn = valueOf("_"+new DecimalFormat("0000").format(n));

    How is valueOf implemented? Is it an internal function of Java/C#/whatever this is?

    Does this likely use a hashtable or rather a linear search?

     

    @DrPepper said:

    Filed under: This post will never change either.
    It will next time the tags are purged.


  • Discourse touched me in a no-no place

    @topspin said:

    @DrPepper said:

    Filed under: This post will never change either.
    It will next time the tags are purged.

    Doesn't even have to be that long. Look again.


  • snoofle has just produced a 1337 post!



  • @snoofle said:

    [...], short of the American financial system completely collapsing and going away.
    Wouldn't be the first time.

    Anyway, this could be reasonably solved by making another class with a static HashSet and a static initialiser that iterates over the values() array of the enum, storing the numeric values of the enum (based on the name() method). But it still feels a bit icky.

     



  • Sounds like a job for a regex!


  • Discourse touched me in a no-no place

    @angrysoul said:

    snoofle has just produced a 1337 post!
    Yawn. And?



  • @PJH said:

    @angrysoul said:
    snoofle has just produced a 1337 post!
    Yawn. And?

    NUMBERS ARE EXCITING



  • @PJH said:

    Yawn. And?
    You don't get it because you're not 1337!



  • @snoofle said:

    Of course, putting the constants (mentioned above) into a HashSet would have been the efficient way to go.

    It would have been a more efficient way, but the efficient way would have been java.util.BitSet. Uses approximately 1/224 of the memory* and less arithmetic per lookup.

    *Or 1/288 on a 64-bit VM. I'm assuming an object header of 8 bytes.


Log in to reply