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...

snoofle:

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:

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:

    public static Decimal GetDecimal(object value, Decimal defaultValue = 0M)
    {
      Decimal result;
      if (value == null || !Decimal.TryParse(value.ToString(), out result))
        return defaultValue;
      else

        return 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

Maybe if the only valid numbers were powers of 2.

joe.edwards:

snoofle:

priceIsValid

I remember that show.

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

joe.edwards:

Maybe if the only valid numbers were powers of 2.

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

_leonardo_:

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.

Evilweasil:

joe.edwards:

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...");

db2:

joe.edwards:

snoofle:

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:

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]

snoofle:

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:

joe.edwards:

snoofle:

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:

snoofle:

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.

snoofle:

             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:

Filed under: This post will never change either.

It will next time the tags are purged.

topspin:

DrPepper:

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:

[...], 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!

angrysoul:

snoofle has just produced a 1337 post!

Yawn. And?

PJH:

angrysoul:

snoofle has just produced a 1337 post!

Yawn. And?

NUMBERS ARE EXCITING

PJH:

Yawn. And?

You don't get it because you're not 1337!

snoofle:

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.