3
\$\begingroup\$

Just want to know if this is safe way to clone arrays and plain objects avoiding infinite loops trigger by circular refrences:

//
!(( function ( methodName, dfn ) {
    // add global 'clone' method identifier
    this[methodName] = dfn();
} ).call(
self,
"clone",
function () {

    var _api = {

        // two arrays to keep track of what was cloned
        originalValues : [],
        clonedValues   : [],

        clone   : function ( input ) {
            throw new Error(
                "Failed to clone value: " + input + "."
            );
        },

        ownEach : function ( obj, func ) {
            for ( var p in obj ) {
                obj.hasOwnProperty( p )
                && func.call( obj, p, obj[p] );
            }
            return obj;
        }
    };

    // helper functions
    function corstr( o ) {
        return Object.prototype.toString.call( o );
    }
    function corslice( args, i1, i2 ) {
        return Array.prototype.slice.call( args, i1, i2 );
    }
    function isArray( o ) {
        return corstr( o ) === "[object Array]";
    }
    function isObject( o ) {
        return corstr( o ) === "[object Object]";
    }
    function bind( obj, fn ) {
        return function () {
            return fn.apply( obj, corslice( arguments ) );
        };
    }
    function override( obj, method_v1, method_v2 ) {
        var coreMethod  = obj[method_v1];
        return obj[method_v1] = function () {
            return method_v2.apply(
                this,
                [ bind( this, coreMethod ) ]
                .concat( corslice( arguments ) )
            );
        };
    }

    //
    // build up clone method

    // handles arrays and objects
    override(
        _api,
        "clone",
        function ( oldMethod, input ) {
            var cloned, isarray = false;
            if (
                isArray( input )
                && ( isarray = true, cloned = [] )
                || isObject( input )
                && ( cloned = {} )
            ) {
                _api.originalValues.push( input );
                _api.clonedValues.push( cloned );
                isarray
                && (
                  input
                  .forEach(
                    function ( val ) {
                        cloned.push( _api.clone( val ) );
                    }
                  ),
                  true
                )
                || (
                  _api.ownEach(
                    input,
                    function ( prop, val ) {
                        cloned[prop] = _api.clone( val );
                    }
                  )
                );
                return cloned;
            } else {
                return oldMethod( input );
            }
        }
    );

    // copies anything else
    override(
        _api,
        "clone",
        function ( oldMethod, input ) {
            // here checks if value has been already cloned
            // and returns it if it has
            var k;
            if ( ( k = _api.originalValues.indexOf( input ) ) != -1 ) {
                return _api.clonedValues[k];
            }
            if (
                !isArray( input )
                && !isObject( input )
            ) {
                _api.originalValues.push( input );
                _api.clonedValues.push( input );
                return input;
            } else {
                return oldMethod( input );
            }
        }
    );

    return function ( value ) {
        var out = _api.clone( value );
        // empties arrays after _api.clone() func is done
        _api.originalValues.length
        = _api.clonedValues.length
        = 0;
        return out;
    };
}
));

// test

var o  = [];
var clon;

o.push( o );
o.push( { p : o } );

clon = clone( o );

console.log( o );
console.log( clon );
console.log( 'o === clon -> '          , o      === clon      );
console.log( 'o[0] === clon[0] -> '    , o[0]   === clon[0]   );
console.log( 'o[1] === clon[1] -> '    , o[1]   === clon[1]   );
console.log( 'o[1].p === clon[1].p -> ', o[1].p === clon[1].p );
//
//  [[...], Object { p=[2]}]
//  [[...], Object { p=[2]}]
//  o === clon           -> false
//  o[0] === clon[0]     -> false
//  o[1] === clon[1]     -> false
//  o[1].p === clon[1].p -> false
//
\$\endgroup\$
5
\$\begingroup\$

What the tin says

Your code seems to track all cloned objects, so that it will find circular references, so it should be bullet proof indeed.

The goggles!

!(( function ( methodName, dfn ) {
    // add global 'clone' method identifier
    this[methodName] = dfn();
} ).call(
self,
"clone",
function () {
  • There seems to be no good reason to negate the outcome of that function
  • Splitting function parameters over different lines should be avoided unless it makes your code more readable, that is definitely not the case here! You do this several times in the code.

Also this:

    _api.originalValues.length
    = _api.clonedValues.length
    = 0;

looks so much better as

_api.originalValues.length = _api.clonedValues.length = 0;

finally, stuff like this:

            && (
              input
              .forEach(
                function ( val ) {
                    cloned.push( _api.clone( val ) );
                }
              ),
              true
            )

Might have some artistic value, but it makes your code far too hard to grok.

Overkill

clone   : function ( input ) {
  throw new Error(
    "Failed to clone value: " + input + "."
  );
},

I assume you are very proficient in another OO language, but things like creating a useless clone function have absolutely no value in JavaScript, but it can eat up bandwidth and cpu time.

Naming

  • You are not consistent with lowerCamelCase
  • I understand why you call these corstr and corslice, they are still bad names
  • Having a function isArray and a var isarray is mildly evil

Checking on Arrays and Objects

I really like this implementation:

function isType(object, type){
     return object != null && type ? object.constructor.name === type.name : object === type;
}

Then you can

if( isType( input , Array ) )

Scope

return function ( value ) {
    var out = _api.clone( value );
    // empties arrays after _api.clone() func is done
    _api.originalValues.length
    = _api.clonedValues.length
    = 0;
    return out;
};

is wrong, why should the caller of clone need to worry about originalValues and clonedValues ? These should be part of clone so that no house keeping is required. Then you can simple have

return _api.clone;

Advanced Hackery

  • Assignments inside if statements are cool, but should only be used in personal projects. They are a source of bugs and confusion for other developers.
  • Writing you own bind is great, but I would suggest you check out the bind shim of Mozilla.

Finale

I would not want to work with this code until it got beautified and all jshint trouble fixed. It works, but I am certain that a far shorter and far more readable implementation can be written.

\$\endgroup\$
  • \$\begingroup\$ I agree, a lot of cleanup can be made, I'm still figuring out the 'best code style' to stick to in JavaScript, because it's very free form and 'do it your self' language. I needed some sort of confirmation that the aproach: 'check what you got so far cache new values you come accross', successfully handles copying self-references without getting stuck. Code snippet is just part of tree data structure implementation I'm working on, where greatest challenge I faced was cloning nodes wich can hold dozen of direct and indirect cyclic references. \$\endgroup\$ – Nikola Vukovic Jan 24 '14 at 22:57
  • \$\begingroup\$ I don't see why scope is wrong as you say, internal clone method does it's job and cleans after itself, returns copied object. Caller doesn't need to care about internal logic because returned wrapper function does that. \$\endgroup\$ – Nikola Vukovic Jan 24 '14 at 23:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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