3
\$\begingroup\$

I'm looking for a code review for my JavaScript code that checks if there is any memory leaks in data structures. Please include constructive criticism, as well as details on what I can do to make this code more efficient.

// memoryleak.js
// (C) 2014 Kyle Maune
// checks for memory leaks in data structs

MemoryLeak = {

  uniq_id: (new Date()).getTime(),
  checked: 1,
  is_seen: [],

  checkLeaks: function(obj) {
    var self = MemoryLeak

    if(!obj || (typeof obj == 'function') || self.checked > 20000)
      return;

    if ((self._isArray(obj) || self._isObject(obj))) {
      if (self._isArray(obj)) {
        self._logTooBig(obj, obj.length)
        for (var i = 0; i < obj.length; i++) {
          self._checkIfNeeded(obj[i])
        }
      }
      else if (self._isObject(obj)) {
        self._logTooBig(obj, self._keys(obj).length)

        for (var key in obj) {
          self._checkIfNeeded(obj[key])
        }
      }
    }
  };

  _checkIfNeeded: function(obj) {
    if (!obj)
      return;

    var self = MemoryLeak;
    self.checked++

    if ((self._isArray(obj) || self._isObject(obj))) {
      if(obj.__leaks_checked == self.uniq_id)
        return;
      obj.__leaks_checked = self.uniq_id

      setTimeout(self._partial(self.checkLeaks, obj), 5);
    }
  };

  _logTooBig: function(obj, limit) {
    if (limit > 200) {
      console.log('Object too big, memory leak? [size: ' + limit + ']')
      console.log(obj)
      console.log('-------')
    }
  };

  _keys: function(obj) {
    var rval = [], prop
    for (prop in obj)
      rval.push(prop)
    return rval;
  };

  _isArray: function(obj) {
    try {
      return obj instanceof Array
    }
    catch(e) {
      return false;
    }
  };

  _isObject: function(obj) {
    return (typeof obj == 'object')
  };

  _partial: function(fn) {
    var args = Array.prototype.slice.call(arguments)
    args.shift()
    return function() {
      var new_args = Array.prototype.slice.call(arguments)
      args = args.concat(new_args)
      return fn.apply(window, args)
    }
  }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I don't understand how this works. Do you have an example program that triggers a memory leak warning? \$\endgroup\$ – 200_success Jun 9 '14 at 17:11
  • \$\begingroup\$ What in the heck are you trying to do here? I think you need to explain the goal of this code (e.g. how it works) and then perhaps we can offer an opinion on whether it is accomplishing that particular goal. Have you just hard-coded some arbitrary limit of 200 keys on an object and you consider that to be a leak if there are more than 200 keys on an object or elements in an array? \$\endgroup\$ – jfriend00 Jun 9 '14 at 17:18
  • \$\begingroup\$ @jfriend00 yeah, that was my goal. I hard coded an arbitrary limit of 200 keys and anything more than 200 keys on an object is considered a leak. FYI I am new to JavaScript. \$\endgroup\$ – user44869 Jun 9 '14 at 17:40
  • \$\begingroup\$ @200_success ATM no. This is hard coded so that an object has a limit of 200 keys and anything more is considered a leak. I'm new to JavaScript as well, please bare w/ me \$\endgroup\$ – user44869 Jun 9 '14 at 17:42
  • 1
    \$\begingroup\$ It would be a pretty narrowly defined program where measuring the number of keys on an object or the length of an array would be a meaningful measure of memory consumption. This is rarely where memory leaks occur in a javascript program and the more general purpose way to measure memory consumption is by using tools built into the browser. See the Heap Profiler for Chrome. \$\endgroup\$ – jfriend00 Jun 9 '14 at 17:48
1
\$\begingroup\$

This code

if ((self._isArray(obj) || self._isObject(obj))) {
  if (self._isArray(obj)) {
    self._logTooBig(obj, obj.length)
    for (var i = 0; i < obj.length; i++) {
      self._checkIfNeeded(obj[i])
    }
  }
  else if (self._isObject(obj)) {
    self._logTooBig(obj, self._keys(obj).length)

    for (var key in obj) {
      self._checkIfNeeded(obj[key])
    }
  }
}

should be reduced, remove the outside if statement

  if (self._isArray(obj)) {
    self._logTooBig(obj, obj.length);
    for (var i = 0; i < obj.length; i++) {
      self._checkIfNeeded(obj[i]);
    }
  }
  else if (self._isObject(obj)) {
    self._logTooBig(obj, self._keys(obj).length);

    for (var key in obj) {
      self._checkIfNeeded(obj[key]);
    }
  }

There is no need for it.


Also some of your one liner blocks make use of Curly Brackets and some do not, this is confusing when put together with your 2-space indenting instead of 4-space indenting. don't worry about space when writing your JavaScript, use all the characters that you need to be able to maintain the code properly, don't Minify until you are completely finished with the code.

Especially when nesting if statements

if ((self._isArray(obj) || self._isObject(obj))) {
  if(obj.__leaks_checked == self.uniq_id)
    return;
  obj.__leaks_checked = self.uniq_id

  setTimeout(self._partial(self.checkLeaks, obj), 5);
}

you should use brackets and proper indenting to make it clear what is going on

if ((self._isArray(obj) || self._isObject(obj))) {
    if(obj.__leaks_checked == self.uniq_id){
        return;
    }
    obj.__leaks_checked = self.uniq_id;

    setTimeout(self._partial(self.checkLeaks, obj), 5);
}

This is also one that almost confused me as well

_keys: function(obj) {
  var rval = [], prop
  for (prop in obj)
    rval.push(prop)
  return rval;
};

I would write it like this (and don't forget the ;)

_keys: function(obj) {
    var rval = [], prop;
    for (prop in obj){
        rval.push(prop);
    }
    return rval;
};

I just realized how many times you left out the semi-colons after a line of code, this is a very bad habit to fall into.

Always remember your semi-colons!

There are places where I don't think the code will run correctly because you don't have semi-colons to tell the browser that you have ended the command.

\$\endgroup\$
  • \$\begingroup\$ Thank you for taking the time to review my code. I'll be sure to clean up the code and follow your code design recommendations. Thanks again! \$\endgroup\$ – user44869 Jun 9 '14 at 20:10

Your Answer

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