Opened 9 years ago

Closed 9 years ago

#2444 closed New Feature (fixed)

V3: Utility functions

Reported by: Frederico Caldeira Knabben Owned by: Frederico Caldeira Knabben
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 (8)

comment:1 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review? added
Owner: set to Frederico Caldeira Knabben
Status: newassigned

The function defined in that object should be reviewed.

comment:2 Changed 9 years ago by Frederico Caldeira Knabben

Type: BugNew Feature

comment:3 Changed 9 years ago by Frederico Caldeira Knabben

Summary: V3: Utility functionV3: Utility functions

comment:4 Changed 9 years ago by Martin Kou

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 ; Changed 9 years ago by Frederico Caldeira Knabben

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 9 years ago by Frederico Caldeira Knabben

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 9 years ago by Martin Kou

Keywords: Review+ added; Review? removed

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

comment:8 Changed 9 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy