Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

My code will check if the variable myString has 20 running numbers. I'm new to JavaScript and I'm wondering if there is a simpler way to do this. How can I shorten my code and possibly make it more efficient?

var myString = "tuxwuhkocx14789470019215498263ljmgwsxvne";
var runningNumbers = 0;

for (i = 0; i < myString.length; i++) {

    var character = myString.charAt(i);

    if (!isNaN(character)) {
        // If character is a number:
        runningNumbers = runningNumbers + 1;
    } else {
        // If character is NOT a number:
        runningNumbers = 0;
    }

    if (runningNumbers == 20) {
        console.log("myString has 20 running numbers.");
    }

}

Output:

myString has 20 running numbers.

share|improve this question
3  
What should happen when there are more than 20 numbers in a row? – Ivan Modric Feb 18 at 8:36
    
@IvanModric, it will print the message again for each extra number past 20. – SparK Feb 19 at 11:15
1  
I think it should be called digits, because 10, -1 and 0.5 might also be considered numbers. – SpaceTrucker Feb 19 at 12:14
    
That string has one number in it. A large one. You should say 'numerals' or 'digits'. – Octopus Feb 19 at 17:19
up vote 31 down vote accepted

If you use a regular expression to look for 20 digit characters (\d) in a row, then the code would be compact and the goal would be visible at a glance:

var twentyDigits = /\d{20}/;
if (twentyDigits.test(myString)){
  console.log("My string has 20 running numbers");
}

You can shorten this further as just:

if (/\d{20}/.test(myString)) console.log("Yup. 20");
share|improve this answer
4  
Please mention that it can also be much faster this way as REs can in many cases just skip the input they don't care about - in this case, if you find a non-digit, you can easily skip 20 chars in advance. – d33tah Feb 19 at 11:37

It's simpler to use a regex as @Phrogz already pointed out.

There are other issues too in the posted code that should be pointed out and improved.

After finding 20 consecutive digits, the program will continue counting, pointlessly. I'm wondering if this was really your intention. It seems that perhaps you want to break out of the loop after finding 20 and stop searching.

Also note that you could move the condition on runningNumbers == 20 right after runningNumbers is incremented. When runningNumbers was not changed (character is not a digit), it's pointless to check its value (you know it's 0).

Instead of printing in a loop, I recommend to rewrite your test as a function returning a boolean value, true if a sequence was found. This way the responsibilities of searching and printing will be well separated.

As a tip on style, note that instead of x = x + 1, you can use the shorter x += 1 or ++x or x++.

share|improve this answer
    
Correct me if I'm wrong, but doesn't use of the RegEx method (instead of the code in the question) resolve all the bugs you've pointed out? – Iszi Feb 18 at 16:51
11  
@Iszi it resolves those issues, but the poster might make those kind of mistakes again in other code if we don't point out and warn against them now. Also this is Code Review SE, not Alternative Implementation SE. – janos Feb 18 at 17:45
    
Next time through the loop after finding 20 digits, wouldn't it increment to 21 and no longer output? If he were doing >= 20 then, yeah. – GalacticCowboy Feb 19 at 17:58
    
@GalacticCowboy oh crap, you're right, well spotted. Updated now, thanks! – janos Feb 19 at 18:19

The regular expression /\d{20}/, as suggested by @Phrogz, is definitely the simplest and most idiomatic way to do this in JavaScript.

Since this is Code Review, I'm obligated to point out that you neglected to write var i, which is a likely cause of future bugs.

I also suggest using a conditional expression

runningNumbers = isNaN(myString[i]) ? 0 : runningNumbers + 1;

to emphasize the fact that one way or another, runningNumbers will get a new value.

share|improve this answer

As for efficiency, you can significantly reduce the number of characters you need to check by only checking every 20th character - you are guaranteed to hit every sequence of 20 consequtive numbers.

As so:

For every 20th character:
    if character is a number:
        length = 1
        loop backwards to count numbers (add to length)
        loop forwards to count numbers (add to length)
        if length >= 20, break out with positive result

Whether this actually performs better than lets say using the regex method, youd need to measure that (I dont know how things compare in javascript...).

If Im thinking this correctly, this should be beneficial as long as over 1/20 of the characters are not numbers. In reality its probably higher due to the increased complexity of counting the 20 numbers (looping backwards and all...)

Works for any length of number sequence, of course.

If efficiency is not a concern, or if this actually performs worse in the real world, one of the shorter solutions is probably preferable.

share|improve this answer
1  
CPUs read memory in whole cache line sized chunks. Skipping over bytes should not improve performance if the skip size is less than a whole cache line, assuming the loop is simple to execute and most of the time is spent in cache misses. However, this improvement would be great for looking for things bigger than a cache line. – doug65536 Feb 18 at 19:42
    
doug65536 , that may or may not be true. I was assuming javascript would generally be slowish, although some implementations might be fast enough to be memory bound (?). So checking if each character is a number and branching on that could be expensive, maybe even looping itself could be. Besides, think of the poor users who have intel hyperthreading which can take advantage of the latency gaps while other code is waiting for memory ;) (and cache misses shouldnt happen as cpu can predict linear reads usually) – Waterlimon Feb 19 at 8:25

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.