This project is archived and is in readonly mode.

#936 ✓wontfix
Jeremy Fleischman

toInt bug leads to Date.diff bug

Reported by Jeremy Fleischman | June 22nd, 2010 @ 11:54 PM | in 2.0 (closed)

On line 727 of mootools-1.2.4-core.js, the implementation of toInt is as follows

toInt: function(base){
    return parseInt(this, base || 10);
}

However, Javascript's parseInt() doesn't deal nicely with strings such as "1e2". So "1e2".toInt() = 1, when it should be 100.

I noticed this when attempting to diff() two nearly identical Dates, for example:
new Date(1277244682000).diff(new Date(1277244682237)) = 7, when these dates are fractions of a second apart.
This is because:

new Date(1277244682237)-new Date(1277244682000) = 237.
-> 237/(1000*60*60*24*365) = 7.515220700152207e-9
-> parseInt(7.515220700152207e-9) = 7

Since Javascript's parseFloat() seems to deal fine with scientific notation, I propose changing the definition of toInt to

toInt: function(base){
    return Math.floor(this.toFloat());
}

This has solved the problem for me.

Comments and changes to this ticket

  • Michael Ficarra

    Michael Ficarra June 23rd, 2010 @ 05:49 AM

    • Tag changed from toint to date, diff, toint

    In your proposed solution, you have completely ignored the fact that numbers of an arbitrary base can be parsed with String.toInt(). Your example of "1e2" can be interpreted as 482 when assuming a base of 16. Though I agree there may be a problem with Date.diff(), String.toInt() is not flawed here ans should not be altered.

  • Jeremy Fleischman

    Jeremy Fleischman June 23rd, 2010 @ 11:03 AM

    You're right, I did completely forget about bases other than 10. I also didn't realize that there are 2 implementations of toInt() in the mootool's core, one for numbers and the other for strings.

    However, that doesn't change the fact that mootools's current implementation of toInt() doesn't seem to work with numbers less than 1e-7. This is not really mootools's fault, as it would appear that the implementation of parseInt() doesn't work with these small numbers either. I do think that mootools's should deal with this, however.

    >>> parseInt(0.0000001)
    1
    >>> (0.0000001).toInt()
    1
    

    As a related side question, why does Number.toInt() take a base as an argument? A number doesn't have a base, it's just a series of 1's and 0's sitting in memory somewhere. It only gets a base once you call its toString() method.

    >>> (10).toInt(2)
    2
    

    The output above just doesn't make sense to me.

    I feel that Number.toInt() should be implemented as follows (I forgot about negative numbers in my first post):

    toInt: function(base){
        return this | 0; //this has the effect of truncating the fractional part of this number
    }
    

    Just to be clear, my earlier proposal was to change String.toInt(), which as Michael pointed out, doesn't have any flaws. However, I'm fairly certain that Number.toInt() is indeed flawed.

  • Michael Ficarra

    Michael Ficarra June 23rd, 2010 @ 02:46 PM

    Number.toInt() does make sense. Think of it as a shortcut for ().toString().toInt([base]). I also don't see any problems with the implementation. It allows base conversion, unlike your proposed change, and uses a bultin function, which I am assuming would be faster than anything we would write, since it can be implemented natively.

    Also, for future reference, try using the shifting bitwise operators. To truncate the fractional part of a float, use << 0. To ensure a number is a positive integer, use >>> 0. << is also useful for calculating powers of two very quickly (1 << 5 == 32).

    I suggest marking this ticket as invalid. Thanks for the ticket, though, Jeremy.

  • Jeremy Fleischman

    Jeremy Fleischman June 23rd, 2010 @ 11:33 PM

    Thank you for the quick reply once again.

    I understand that toInt() is a shortcut for toString().toInt(), but my earlier point was that I can't think of any conceivable use case for Number.toInt() with a base other than 10 (just to be clear that I know what's going on, I understand that having a String.toInt(base) is absolutely necessary). I'd argue that anyone trying to call Number.toInt(10) with a base other than 10 probably doesn't realize they're doing that, and it would be better to error out (or at least ignore the base argument) than do what I consider some internal magic. However, the behavior of Number.toInt() with a base argument was not the point of this ticket, and if you choose to leave its current behavior, that would be fine by me, except that ...

    The point of this ticket was to fix an issue with Date.diff() that I believe is caused by the bad behavior of Number.toInt() when dealing with (in my experience) numbers less than 1e-7. You have said that you don't see any problems with the current implementation of Number.toInt(), but you haven't explicitly commented on the fact that it simply doesn't work with a number such as 1e-7. Do you feel that the current behavior is acceptable? It's certainly breaking Date.diff(), and would also break any other part of mootools that uses Number.toInt().

    Thank you for your consideration of this ticket, I'm really enjoying learning mootools for this project I'm working on, and it would be awesome if Date.diff() worked out of the box for me.

  • Michael Ficarra

    Michael Ficarra June 24th, 2010 @ 01:14 AM

    I see where you are coming from and had forgotten that I did acknowledge that there was a problem while using Date.diff(). Try out these solutions:

    String.implement('toInt',function(base){
        return parseInt(this.indexOf('e')>=0 ? parseFloat(this)<<0 : this, base);
    });
    Number.implement('toInt',function(base){
        return parseInt(String(this).indexOf('e')>=0 ? parseFloat(this)<<0 : this, base)
    });
    

    They seem to work for me. I would recommend these patches be implemented. They are not breaking and will improve support for very small and very large numbers.

  • Jeremy Fleischman

    Jeremy Fleischman June 24th, 2010 @ 05:35 AM

    Awesome, I'm glad you agree there's something that needs to be fixed here. I think you made the same mistake I did at first and forgot about base >= 15, which use the letter "e" as a digit. Your current implementation of String.toInt() gives me this

    >>> "e".toInt(16)
    0
    
    That can't be right.

    Your Number.toInt() doesn't suffer from this problem, although I do believe that it should ignore the base entirely and just return something like (this | 0) or (this << 0). It would certainly be a lot faster than your implementation, or the current implementation, which rely upon calling toString() and then parseFloat/Int.

    Lastly, javascript allows a capital "E" for scientific notation, so you should probably switch all instances of foo.indexOf('e') to either foo.toLowerCase().indexOf('e') or foo.indexOf(/e/i). I'm not sure which of these is faster, although I'd guess it would be best to ignore regexes entirely.

  • Jeremy Fleischman

    Jeremy Fleischman June 24th, 2010 @ 05:36 AM

    I couldn't find an edit post option, so just to be clear, in the last sentence above I meant "avoid regexes", not "ignore regexes".

  • Michael Ficarra

    Michael Ficarra June 24th, 2010 @ 06:18 AM

    Agh, you are right, I completely forgot about that. Here's a slightly less elegant, yet functionally correct update:

    String.implement('toInt',function(base){
        base = base > 1 ? base : 10;
        if(base>14) return parseInt(this,base);
        return parseInt(this.search(/e/i)>=0 ? this.toFloat()<<0 : this, base);
    });
    Number.implement('toInt',function(base){
        return this.toString().toInt(base);
    });
    

    After experiencing these toInt inadequacies first-hand, I am much more confident that the current implementation should be considered a defect. Unfortunately, my proposed solution is a little slower than the current implementation, but I believe the trade-off is worth it.

  • Jeremy Fleischman

    Jeremy Fleischman June 24th, 2010 @ 06:29 AM

    Ok, I like where this is going.

    You never told me a valid use case for Number.toInt() with a base other than 10, but I can understand not wanting to break backwards compatibility. However, since I think most uses of Number.toInt() will be with no base, I'd like to see this minor change to your Number.toInt(), which is functionally identical, and will (I assume) run a whole lot faster.

    Number.implement('toInt',function(base){

    if(!base || base == 10)
        return this &lt;&lt; 0;
    return this.toString().toInt(base);
    
    
    
    
    });

    Your String.toInt(), while unfortunately complicated, does seem find to me. Is there any reason why you used this.search instead of this.indexOf, though?

  • Michael Ficarra

    Michael Ficarra June 24th, 2010 @ 06:47 AM

    I agree that your Number.toInt() improvement would be faster on most occasions, so that or something similar should probably be implemented.

    The reason for String.search() instead of String.indexOf() is to allow the case-insensitive regex instead of calling String.toLowerCase() every time. Most browsers' regex searches are pretty optimized by now, and IE has a long-running tradition of performing particularly slowly in string operations (such as String.toLowerCase()). If you would like to do benchmarks to prove otherwise, I'd recommend a change, but I have a hunch the regex operation will be faster.

  • Jeremy Fleischman

    Jeremy Fleischman June 24th, 2010 @ 06:52 AM

    I trust you when you say that search() is faster than toLowerCase(). I thought that you could pass a javascript regex to indexOf (something like this.indexOf(/e/i), which I just checked that you can't do, so of course you must use String.search(). Pardon me!

  • Michael Ficarra

    Michael Ficarra June 24th, 2010 @ 07:03 AM

    A patch containing the changes that we discussed is attached.

  • Michael Ficarra

    Michael Ficarra June 24th, 2010 @ 04:16 PM

    • Tag changed from date, diff, toint to number, string, toint

    I have made a minor change, in which 14 was changed to 0xE for clarity. It is a less arbitrary-looking constant, and should hopefully be more self-explanatory to people reading the source. It is a single character longer, but the compilers should convert it to its decimal representation. This patch should be applied on top of the last one.

  • Christoph Pojer

    Christoph Pojer July 11th, 2010 @ 10:26 PM

    • Milestone set to 2.0
    • State changed from “new” to “open”
    • Milestone order changed from “0” to “0”

    I'm not sure whether we are likely to include this functionality.

    Concerning the base parameter in Number.toInt: parseInt works with numbers the same as it does with strings. That means, that it treats parseInt(10, 2) as binary, which is why it returns 2. Similarly, parseInt(70, 8) returns 56 or parseInt(1000, 16) returns 4096.

  • Christoph Pojer
  • Michael Ficarra

    Michael Ficarra September 5th, 2010 @ 06:30 AM

    Why was this moved to More? This is a bug with String.toInt and Number.toInt, which are both defined in Core.

  • Scott Kyle

    Scott Kyle September 5th, 2010 @ 08:19 AM

    • State changed from “duplicate” to “wontfix”
    • Assigned user set to “Scott Kyle”

    We decided to not going to fix JS parseInt behavior for numbers less than 1e-6, but Date.diff no longer will exhibit this behavior due to this fix:

    http://github.com/mootools/mootools-more/commit/f069084730f77e120c7...

    Hope you find that reasonable :)

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Shared Ticket Bins

Attachments

Referenced by

Pages