Test if Something is an Integer

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

You've got nothing to test if it is a "smallint", "bigint" or other variation.. as posted before;

Code: Select all

preg_match('/^[0-9]+$/', $string)
does exactly what your class does, without the drawbacks your class has.. for a start, you are dependent on ctype - which is an optional extension.

As for the bashing, well this is the Critique forum, it's not the "Pat on the back and a congratulations" forum. :)
User avatar
CoderGoblin
DevNet Resident
Posts: 1425
Joined: Tue Mar 16, 2004 10:03 am
Location: Aachen, Germany

Post by CoderGoblin »

ole wrote:Yeah I would never have been able to write my limerick if OTT wasn't used.
Ah but you see here you do things too small... A limerick is five lines :lol:
And for those you don't know OTT is stands for Over The Top.
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

You've got nothing to test if it is a "smallint", "bigint" or other variation
Yes I do

Code: Select all

public function invoke(array $testSubjects, $signed = false, $maxInt = PHP_INT_MAX)
for a start, you are dependent on ctype - which is an optional extension
It's part of the core, and is faster and more secure that PCRE. But if it makes you happy you could write this:

Code: Select all

private function _unsigned($testSubject)
{
    // its obviously false if it contains non-digit characters
    if (function_exists('ctype_digit')) {
       if (!ctype_digit($testSubject)) {
           return false;
       }
    } else if (!preg_match('/^\d+$/', $testSubject)) {
        return false;
    }
Your regex of /^[0-9]+$/ won't allow signed strings - and before you say neither will mine look at the way _unsigned() is called. Nor will a regex alone fail on 99999999999999999999999999999999 which is obviously too big to be stored anywhere. and for that matter what if PHP is compiled without PCRE? To be honest, checking for the existance of things that I know exist on my server is just adding bloat where it isn't necessary - where as I do use both signed, unsigned, big, small and other types of integers in my databases.

Incidentally, for everyone's information, Jenk does have something personal against me that explains his behaviour.
User avatar
JayBird
Admin
Posts: 4524
Joined: Wed Aug 13, 2003 7:02 am
Location: York, UK
Contact:

Post by JayBird »

ctype is only available by default on PHP installations => PHP 4.2.0.

I think it would help if you listed all the advantages of using this code snippet over the smaller snippets people have suggested that do the same thing...otherwise people will just presume your using way too much code to do something that can be done in 1 line.
jmut
Forum Regular
Posts: 945
Joined: Tue Jul 05, 2005 3:54 am
Location: Sofia, Bulgaria
Contact:

Post by jmut »

How about? Nobody mentioned this one.

Code: Select all

is_numeric()
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post by Chris Corbyn »

jmut wrote:How about? Nobody mentioned this one.

Code: Select all

is_numeric()
Doesn't just check for integers.
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

Aight. The killer feature in this case is making sure the integer doesn't overflow. This is slightly more complicated than one would expect, since counting the number of digits is not sufficient.

Before I start rambling, keep in mind that nearly all databases will gracefully handle insertions of numbers that overflow by clipping them, and if this is a necessary error condition MySQL can operate in Strict mode, which will fail the query if there's an overflow.

Now, in general, the size of the number we are validating is constrained by the number of bytes/bits we're allowed to use. PHP, internally, uses 32-bit integers (4 bytes), so for any numbers that are within this range +/-2147483648 we can use PHP's regular integer comparison functions to determine whether or not 888 will fit in a TINYINT column (it won't).

The main probably I see, as of right now, is that you expect the user to know what the max value is. They probably don't. All they know is that they're validating for a TINYINT: your class needs to account for that. Also, you need some unit-tests. I believe that -128 is a valid value for a signed TINYINT, but 128 is not.
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Post by Maugrim_The_Reaper »

ctype is only available by default on PHP installations => PHP 4.2.0.
Ctype is available in approximately 95% of all PHP installations vs 97% for PCRE. PCRE is also entirely optional (oh yes it is!) and was only available by default since PHP 4.2.0. Along with a lot of extensions oddly enough... If a PHP installation does not have ctype enabled than it needs updating...not pandering.
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

The killer feature in this case is making sure the integer doesn't overflow. This is slightly more complicated than one would expect, since counting the number of digits is not sufficient.
Are you implying my my code insufficient in this regard?
keep in mind that nearly all databases will gracefully handle insertions of numbers that overflow by clipping them, and if this is a necessary error condition MySQL can operate in Strict mode, which will fail the query if there's an overflow
For some reason I wanted to write the most complete integer validation and I just thought "hey if its too big its not going to be an integer" and wrote the validations. I think you guys have done a pretty good job of convincing me that it is not the responsibility of integer validation to ensure an integer is within storable range and if you want to do that there should be a separate numeric range validation (which I have). That keeps thing small, simple and single responsabilitified
we can use PHP's regular integer comparison functions to determine whether or not 888 will fit in a TINYINT column (it won't).
And bcmath for outside of that.. its in my code towards the bottom.
The main probably I see, as of right now, is that you expect the user to know what the max value is. They probably don't. All they know is that they're validating for a TINYINT: your class needs to account for that.
Yeah, that would make it more useful. But, going back to the main issue, it is necessary to do this at all? I always validate a string is less than 255 chars if its going in a varchar because I don't want the user to experience truncation unexpectedly. Perhaps the principle only applies to small integers where an overflow is more likely. And again you probably want a separate range validation because you don't want a "not an integer" error when you really me "number is too large".
Also, you need some unit-tests.
Besides the one I posted?
I believe that -128 is a valid value for a signed TINYINT, but 128 is not.
Yes, you are right.
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

You do a very good job of explaining the objections. Go with your new idea: simplistic integer checking combined with numeric bounds checking.
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

* rubs eyes *

Is it me or did we just reach a unequivocal agreement and the devnet forums?! Maybe there is hope.
Thanks AC.

d11wtq | Noticed this was your 2007th post, in year 2007
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post by RobertGonzalez »

In the spirit of modular coding, shouldn't there be classes for:
  • checking if a value is an integer
  • checking if an integer is within bounds
  • exceptions for both cases
That would keep singular responsibility tied to each class, and decouples the integer checks from the boundary checks. Or am I not reading this correctly?
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

I am going to have separate classes for is integer and within range.
I am not going to throw an exception if a validation fails because I do not believe a failed validation is a particularly serious or unexpected failure.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

Exceptions aren't solely for serious or unexpected errors. They are a facility for passing the error handling to a layer above the current one. That may be the calling layer, but could be higher.
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

Exceptions aren't solely for serious or unexpected errors.
But that is what they are most commonly assoicated with and this is important when you consider that a one of the good reasons for using them is the readability they add.
They are a facility for passing the error handling to a layer above the current one. That may be the calling layer, but could be higher.
And this is probably the only functional advantage to using them and inappropriate, given my current implementation. Presently I'm using a component paradigm to invoke validations and store the error message and its destination, if the destination is missing no error message is displayed, not optimal but better than fatality surely.
Post Reply