Ticket #2444 (closed New Feature: fixed)

Opened 6 years ago

Last modified 6 years ago

V3: Utility functions

Reported by: fredck Owned by: fredck
Priority: Normal Milestone: CKEditor 3.0
Component: General Version:
Keywords: Confirmed V3ProtoBase Review+ Cc:

Description

The CKEditor prototype defines the CKEDITOR.tools object, which holds several utility functions using all around our code.

Function under this object must be totally generic, independent of other objects and classes.

Change History

comment:1 Changed 6 years ago by fredck

  • Keywords Review? added
  • Owner set to fredck
  • Status changed from new to assigned

The function defined in that object should be reviewed.

comment:2 Changed 6 years ago by fredck

  • Type changed from Bug to New Feature

comment:3 Changed 6 years ago by fredck

  • Summary changed from V3: Utility function to V3: Utility functions

comment:4 follow-up: ↓ 5 Changed 6 years ago by martinkou

Is there any reason the *trim functions are written like this?

        trim : (function()
        {
                // We are not using \s because we don't want "non-breaking spaces" to be caught.
                var trimRegex = /(?:^[ \t\n\r]+)|(?:[ \t\n\r]+$)/g;
                return function( str )
                {
                        return str.replace( trimRegex, '' ) ;
                };
        })(),

        /**
         * Remove spaces from the start (left) of a string. The following
         * characters are removed: space, tab, line break, line feed.
         * @param {String} str The text from which remove the spaces.
         * @returns {String} The modified string excluding the removed spaces.
         * @example
         * alert( CKEDITOR.tools.ltrim( '  example ' );  // "example "
         */
        ltrim : (function()
        {
                // We are not using \s because we don't want "non-breaking spaces" to be caught.
                var trimRegex = /(?:^[ \t\n\r]+)/g;
                return function( str )
                {
                        return str.replace( trimRegex, '' ) ;
                };
        })(),

        /**
         * Remove spaces from the end (right) of a string. The following
         * characters are removed: space, tab, line break, line feed.
         * @param {String} str The text from which remove the spaces.
         * @returns {String} The modified string excluding the removed spaces.
         * @example
         * alert( CKEDITOR.tools.ltrim( '  example ' );  // "  example"
         */
        rtrim : (function()
        {
                // We are not using \s because we don't want "non-breaking spaces" to be caught.
                var trimRegex = /(?:[ \t\n\r]+$)/g;
                return function( str )
                {
                        return str.replace( trimRegex, '' ) ;
                };
        })()

There's no question it works, but they can certainly be simplified.

comment:5 in reply to: ↑ 4 ; follow-up: ↓ 6 Changed 6 years ago by fredck

Replying to martinkou:

Is there any reason the *trim functions are written like this?

This is just a micro-optimization...

At runtime, regular expressions are parsed and compiled before run. Defining them as private variables in a closure make it work like a "cache" as the parsing and compilation is done only once.

Also, looking at your comment, just came to my eyes that th ltrim and rtrim regexes have extra stuff copied from trim that is not needed. I've fixed it with [2347].

comment:6 in reply to: ↑ 5 Changed 6 years ago by fredck

Replying to fredck:

This is just a micro-optimization...

Another note on this.

It doesn't mean that we should always do such optimizations. We must consider the benefits of it, over code simplification.

In the case of the string trimming functions, they participate (a lot) in the HTML ouput generation. I wanted to have as much performance as possible there, so I've considered all kinds of optimizations.

Of course, we would not have much benefit from it if those functions would be called just a few times. In that case, simpler code would be more important than such minimal performance enhancement.

comment:7 Changed 6 years ago by martinkou

  • Keywords Review+ added; Review? removed

Alright, the optimization makes sense since *trim() gets called a lot. Thanks for the clarification.

comment:8 Changed 6 years ago by fredck

  • Status changed from assigned to closed
  • Resolution set to fixed
Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy