• Christian (unregistered) in reply to Double-Posting Guy
    Double-Posting Guy:
    At what time does Alex post articles on this so-called "daily" site?
    It's more ironic... Like The Daily Show with Jon Stewart.
  • AnonymousCoward (unregistered)

    Wouldn't empty strings be accepted too ?

  • (cs) in reply to Double-Posting Guy
    Double-Posting Guy:
    At what time does Alex post articles on this so-called "daily" site?
    That reminds me...The "Alex is dead" guy hasn't been seen recently. Is he on vacation or something?
  • not frits at all (unregistered)

    Are you saying that the "Alex is dead" guy is dead ?

  • Sarcastic kebab reaper (unregistered)

    Imagine this thing is written in PHP Half of the comments is about how much PHP sucks.

  • (cs) in reply to not frits at all
    not frits at all:
    Are you saying that the "Alex is dead" guy is dead ?
    I would, but I don't have any creative stories about how he died. Maybe one of the others can assist?
  • Jay (unregistered)

    Wow, in that short code snippet, and three different ways to represent true vs false! ("Y"/"N", "1"/"0", and the boolean) 'Hate to get bogged down in consistency or anything.

    Still, it could have been worse. At my last job I worked on a system where the original developers used the strings "true" and "false". I came across a bug once where they had written, if (flag.equals("flase")), i.e. they had a typo spelling "false". As of course "false".equals("flase") is false, false looked like true, with unproductive results.

    The same programmer stored all his numbers as strings, too. When he had to do arithmetic, he converted to Double, did the arithmetic, then converted the result back to a string.

  • qbolec (unregistered) in reply to SG_01
    SG_01:
    return destination.All(x => char.IsNumber(x));
    Is C# really so bad with eta-reduction that you can't simply write:
    return destination.All(char.IsNumber);
    
    ?

    Anyway, some of you miss that spaces, hash, stars or hypens could be added to the list of legal characters in a phone number at some point in future, and that even O(n*m^2) algorithm is fair enough for n,m < 20, as long as it is easier to write, test, and maintain than O(n) one. But I really doubt this one is one of those.

  • Jay (unregistered)

    When I was in school, I was taught that writing

    if (c>='0' && c<='9')
    

    is wrong. The book said that this relies on the assumption that the character set represents digits with contiguous values, i.e. that if we arranged the characters in order, we wouldn't have letters or punctuation marks interspersed with digits. (This isn't unthinkable. See, "EBCDIC".) The "right" way to do it, the textbook informed us, was to create an array and loop through the elements comparing to each.

    Interesting argument, but it seems to me that any code you write makes assumptions about how the computer works. Putting values in an array assumes that the elements in the array are stored contiguously, so that if I loop through them none will be skipped. It assumes that comparing a character stored in a scalar to a character stored in an array is a valid operation. Etc etc.

    Okay, I'll give that textbook author this: There's a difference between relying on things documented as part of the language spec, and relying on things that could at least theoretically be different if you recompiled the code on a different platform.

  • David (unregistered) in reply to @Deprecated

    You missed one:

    • Uses substring instead of an indexer
  • Daniel (unregistered) in reply to @Deprecated

    You danced around the most important fail: it's an O(n^2) solution that could easily be solved in linear time. Most of the other WTFs in that code stem from that problem.

  • Daniel (unregistered) in reply to Daniel

    Blegh, forgot that both loops aren't working on the same set, so it's actually O(nm), and since the valid character array is a constant size, it technically simplifies to O(n), but it's still extremely inefficient.

  • AGray (unregistered) in reply to PedanticCurmudgeon
    PedanticCurmudgeon:
    not frits at all:
    Are you saying that the "Alex is dead" guy is dead ?
    I would, but I don't have any creative stories about how he died. Maybe one of the others can assist?

    Toothbrushing accident.

    These things happen...

    CAPTCHA: praesent - It's not a present.

  • (cs)

    I hereby rechristen this site "The Demiweekly WTF".

  • Friedrice the Great (unregistered) in reply to PedanticCurmudgeon
    PedanticCurmudgeon:
    not frits at all:
    Are you saying that the "Alex is dead" guy is dead ?
    I would, but I don't have any creative stories about how he died. Maybe one of the others can assist?

    Well, turns out that the Alex is Dead guy was really a particular Flame infestation on one of the computers in the DailyWTF office. Then a Flame command system sent the instruction to delete itself ...

  • Enterprise Coder (unregistered) in reply to @Deprecated
    @Deprecated:
    Wow, so many inefficiencies I don't even know where to start!
    • Rebuilds array every time it is called

    Which is fine if it's only called occasionally and reduces the number of globals cluttering the library.

    - Uses an array instead of c >= '0' and c <= '9'

    Some systems may have to deal with EBCIDC, so you can't just assume numbers are in order.

    - Calls substring repeatedly in inner loops

    What's wrong with that? Small objects are efficiently handled by garbage collection, and substring with a fixed length is a constant time function.

    - Does not exit early from outer for loop

    This is clearly a security function, and exiting early would make it vulnerable to side channel attacks.

    - Using strings instead of booleans

    A boolean requires one word... a reference to a literal string requires one word. How is a boolean less efficient?

  • Brian (unregistered) in reply to Mathlete
    private static bool ValidateLinq(string str) { return str.All(x => '0' <= x && x <= '9'); }

    I was surprised that this was quite THAT slow. I know it's not exactly cheap to do the invocation of an anonymous function (which is, iirc, what the lambda gets turned into), but that seems extra slow.

    Maybe using Any() with a negated condition would allow for early exit and speed it up a little?

    Captcha: facilisi. Underwater sealab.

  • Mathlete (unregistered) in reply to Brian
    Brian:
    private static bool ValidateLinq(string str) { return str.All(x => '0' <= x && x <= '9'); }

    I was surprised that this was quite THAT slow. I know it's not exactly cheap to do the invocation of an anonymous function (which is, iirc, what the lambda gets turned into), but that seems extra slow.

    Maybe using Any() with a negated condition would allow for early exit and speed it up a little?

    Captcha: facilisi. Underwater sealab.

    I tried both an the execution times are about the same. FWIW, both Any and All will early exit (as will the regex versions), as can be seen by putting the invalid characters towards the beginning of the string instead of the end:

    		 public static void Main(string[] args)
    		 {
    			 const string valid = "193485923905820394029850298402983409284032840238402984";
    			 const string invalidEnd = "193485923905820394029850298402983409284032840238402984a";
    			 const string invalidStart = "34a534534503985039845038450394850394850398503498503985";
    
    			foreach (var func in new Func<string, bool>[] { ValidateLinqAll, ValidateLinqAny, ValidateRegex, ValidatePrecompiledRegex, ValidateForLoop })
    			{
    				var startTime = DateTime.Now;
    				for (var i = 0; i < 1000000; i++)
    				{
    					func(valid);
    				}
    				var endTime = DateTime.Now;
    				Console.Out.WriteLine(func.Method.Name + " valid: " + endTime.Subtract(startTime).TotalMilliseconds);
    
    				startTime = DateTime.Now;
    				for (var i = 0; i < 1000000; i++)
    				{
    					func(invalidEnd);
    				}
    				endTime = DateTime.Now;
    				Console.Out.WriteLine(func.Method.Name + " invalid: " + endTime.Subtract(startTime).TotalMilliseconds);
    
    				startTime = DateTime.Now;
    				for (var i = 0; i < 1000000; i++)
    				{
    					func(invalidStart);
    				}
    				endTime = DateTime.Now;
    				Console.Out.WriteLine(func.Method.Name + " w/ early exit: " + endTime.Subtract(startTime).TotalMilliseconds);
    			}
    
    			Console.In.ReadLine();
    		}
    
    		private static bool ValidateLinqAll(string str) { return str.All(x => '0' <= x && x <= '9'); }
    		private static bool ValidateLinqAny(string str) { return !str.Any(x => x < '0' || x > '9'); }
    		private static bool ValidateRegex(string str) { return Regex.IsMatch(str, @"^\d*$"); }
    		
    		private static readonly Regex ValidatorRegex = new Regex(@"^\d*$");
    		private static bool ValidatePrecompiledRegex(string str) { return ValidatorRegex.IsMatch(str); }
    
    		private static bool ValidateForLoop(string str)
    		{
    			foreach (var c in str) if (c < '0' || c > '9') return false;
    			return true;
    		}
    

    Results: ValidateLinqAll valid: 613.0613 ValidateLinqAll invalid: 620.062 ValidateLinqAll w/ early exit: 64.0064 ValidateLinqAny valid: 603.0603 ValidateLinqAny invalid: 630.063 ValidateLinqAny w/ early exit: 66.0066 ValidateRegex valid: 2362.2362 ValidateRegex invalid: 3833.3833 ValidateRegex w/ early exit: 788.0788 ValidatePrecompiledRegex valid: 1982.1982 ValidatePrecompiledRegex invalid: 3442.3442 ValidatePrecompiledRegex w/ early exit: 403.0403 ValidateForLoop valid: 85.0085 ValidateForLoop invalid: 86.0086 ValidateForLoop w/ early exit: 11.0011

  • (cs)

    This is great! I'm gonna run right out and make a StringyString class!

  • (cs) in reply to Steve The Cynic
    Steve The Cynic:
    RaceProUK:
    emaN ruoY:
    First, as stated, sometimes RegEx really is the answer.
    Assuming the code is .NET, I think Int32.TryParse would be quicker.
    Nice try, but you need to exclude strings beginning with "-" in some way, as TryParse would presumably say that e.g. "-456" is valid...

    Because checking the output of TryParse to see if it's negative is so complicated...

    Either int.TryParse() or long.TryParse() was my first thought too; you could always use a lazy loop over the string and use the char.IsDigit() or char.IsNumber() methods, depending on what you were actually trying to achieve (IsDigit() checks that the character is strictly a radix-10 digit, IsNumber() checks whether the character is classified by Unicode as a number).

  • BenJoe (unregistered)

    Public Function IsValid(value as string) as boolean Return IsNumeric(value) End Function

    There, VB.NET fixed it :P

    PS: I Know The Wrapping Function is kinda obsolete, but this way it gives more Context here ;)

  • (cs)

    Having recently worked with a company providing this kind of service ... it is unsurprising...

  • Pero Perić (unregistered) in reply to Jay
    Jay:
    ... Putting values in an array assumes that the elements in the array are stored contiguously, so that if I loop through them none will be skipped. ...
    If by array you mean true array, T[] thingy, than coder has control over where elements are "added" and only assumption is that array filling code works correctly. On the other hand, if by array you mean List<T> than you are talking about interfaces and contracts. List<T> may store elements in any way it sees fit but methods and indexers that expose those elements must work the docs says and as you said, in that case assumption is that docs are reliable.

    This comment assumes that we talk about C# and .Net :)

    captcha: validus - your argument is invalidus

  • TheLurker (unregistered) in reply to @Deprecated

    Ahhh, should've used MUMPS.
    It'd be a one liner..

    ; If there's anything left over it's not an integer. I $L($TR(DEST, "0123456789")) > 0

    I'll get my coat...

  • Slugart (unregistered) in reply to @Deprecated

    Or they decide to start accepting hex...

  • sagaciter (unregistered)

    What? No frist idiots any more? Looks like I cleaned this place of them.

  • mp (unregistered) in reply to @Deprecated
    • Writes any of this when matching against "^[0-9]*$" would do?
  • blagh (unregistered) in reply to @Deprecated
    Ugh. Maybe the array is used in case sometime in the future, a new number is added between '4' and '5'.

    Well, yeah, you wanna make sure that if there's ever a need to accept numbers after the decimal point, you can add those in too.

  • Benjamin Howarth (unregistered) in reply to @Deprecated

    Here's the super-duper-timesaving method of doing this exact same thing (I'm assuming C# here):

    private string checkChars(string destination) { return (destination.ToCharArray().All(d => Char.IsNumeric(d))); }

    You're welcome.

  • drobnox (unregistered)
    public static bool IsNumeric(this string s)
    {
         return s.All(c => Char.IsNumber(c)); 
    }
    
    
  • Todd (unregistered) in reply to @Deprecated

    Looks like C#, so wow, how about just

    function ContainsOnlyNumbers(string input) { foreach(var s in input) { if (!Char.IsNumber(s)) { return false; } } return true; }

    Quits ASAP and uses presumably fast built-in functions. Alternatively a reg-ex that only accepts numbers should be quite smart.

  • Jenda (unregistered) in reply to Zmee
    Zmee:
    Just wow! And replacing this with a single line:
    return java.util.regex.Pattern.matches("^\\d*$", destination);
    (And yes, it is d* rather than d+; there is no requirement for the string to be non-empty.)
    Well if this has changed to a code golf ...

    return $destination =~ /^\d*$/;

    or rather the whole sub

    sub checkChars { $_[0] =~ /^\d*$/; }

    Not that anyone would bother making a function for anything as trivial as this.

  • MyWay (unregistered)
            public bool checkChar(string destination)
            {
                int p;
                for(int i = 0; i<destination.Length;i++)
                {
                    if (!int.TryParse(destination.Substring(i, 1),out p))
                    {
                        return false;
                    }
                }
                return true;
            }
    </pre>
    
  • Ben (unregistered)

    I think it's just the function name that's misleading. Should have been checkCharsAndSpinWait

  • deleto (unregistered)

    Looks like Martin should have no difficulty achieving some stunning performance gains, if management even cares.

  • Fūzetsu (unregistered) in reply to @Deprecated

    Hah, you would be surprised…

    http://www.scp-wiki.net/scp-033

  • Vince (unregistered)

    Hello,

    It is probably an entry for an obfuscation contest ? ;-)

    Never heard of isInt() ?

  • Bozo (unregistered) in reply to @Deprecated

    Well here's a place to start:

    • apparently unaware that character has an isDigit function.
    • apparently unaware that Java has a regex package. People sometimes gripe about the performance of it, but if it's replacing the code above it can't help being an improvement.
  • Craig (unregistered) in reply to @Deprecated
    @Deprecated:

    Wait, WTF??? bool validChars = true; and then 4 lines later: string matchedChar = "N";

    This is what happens when bad programmer writes ann ill-conceived implementation that doesn't even work, and competent programmer just simply gets it working by throwing in the validChars boolean.

    Alas the business mindset is to "don't touch it now that it's working". I really do wish that weren't the case, because this seriously should have been overhauled.

    Addmittedly, fixing the return type could prove to be a lot more work because who knows how many times the bad programmer has called his Frankenstein.

Leave a comment on “Stringy Sting Verification”

Log In or post as a guest

Replying to comment #384113:

« Return to Article