path: refactor for performance and consistency

Improve performance by:
+ Not leaking the `arguments` object!
+ Getting the last character of a string by index, instead of
  with `.substr()` or `.slice()`

Improve code consistency by:
+ Using `[]` instead of `.charAt()` where possible
+ Using a function declaration instead of a var declaration
+ Using `.slice()` with clearer arguments
+ Checking if `dir` is truthy in `win32.format`
  (added tests for this)

Improve both by:
+ Making the reusable `trimArray()` function
+ Standardizing getting certain path statistics with
  the new `win32StatPath()` function

PR-URL: https://github.com/nodejs/io.js/pull/1778
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>

lib/path.js

@@ -37,6 +37,29 @@ function normalizeArray(parts, allowAboveRoot) {
   return res;
 }
 
+// Returns an array with empty elements removed from either end of the input
+// array or the original array if no elements need to be removed
+function trimArray(arr) {
+  var lastIndex = arr.length - 1;
+  var start = 0;
+  for (; start <= lastIndex; start++) {
+    if (arr[start])
+      break;
+  }
+
+  var end = lastIndex;
+  for (; end >= 0; end--) {
+    if (arr[end])
+      break;
+  }
+
+  if (start === 0 && end === lastIndex)
+    return arr;
+  if (start > end)
+    return [];
+  return arr.slice(start, end + 1);
+}
+
 // Regex to split a windows path into three parts: [*, device, slash,
 // tail] windows-only
 const splitDeviceRe =
@@ -62,9 +85,21 @@ function win32SplitPath(filename) {
   return [device, dir, basename, ext];
 }
 
-var normalizeUNCRoot = function(device) {
+function win32StatPath(path) {
+  var result = splitDeviceRe.exec(path),
+      device = result[1] || '',
+      isUnc = !!device && device[1] !== ':';
+  return {
+    device,
+    isUnc,
+    isAbsolute: isUnc || !!result[2], // UNC paths are always absolute
+    tail: result[3]
+  };
+}
+
+function normalizeUNCRoot(device) {
   return '\\\\' + device.replace(/^[\\\/]+/, '').replace(/[\\\/]+/g, '\\');
-};
+}
 
 // path.resolve([from ...], to)
 win32.resolve = function() {
@@ -99,11 +134,11 @@ win32.resolve = function() {
       continue;
     }
 
-    var result = splitDeviceRe.exec(path),
+    var result = win32StatPath(path),
-        device = result[1] || '',
+        device = result.device,
-        isUnc = device && device.charAt(1) !== ':',
+        isUnc = result.isUnc,
-        isAbsolute = win32.isAbsolute(path),
+        isAbsolute = result.isAbsolute,
-        tail = result[3];
+        tail = result.tail;
 
     if (device &&
         resolvedDevice &&
@@ -147,11 +182,11 @@ win32.resolve = function() {
 win32.normalize = function(path) {
   assertPath(path);
 
-  var result = splitDeviceRe.exec(path),
+  var result = win32StatPath(path),
-      device = result[1] || '',
+      device = result.device,
-      isUnc = device && device.charAt(1) !== ':',
+      isUnc = result.isUnc,
-      isAbsolute = win32.isAbsolute(path),
+      isAbsolute = result.isAbsolute,
-      tail = result[3],
+      tail = result.tail,
       trailingSlash = /[\\\/]$/.test(tail);
 
   // Normalize the tail path
@@ -176,23 +211,19 @@ win32.normalize = function(path) {
 
 win32.isAbsolute = function(path) {
   assertPath(path);
-
+  return win32StatPath(path).isAbsolute;
-  var result = splitDeviceRe.exec(path),
-      device = result[1] || '',
-      isUnc = !!device && device.charAt(1) !== ':';
-  // UNC paths are always absolute
-  return !!result[2] || isUnc;
 };
 
 win32.join = function() {
-  function f(p) {
+  var paths = [];
-    if (typeof p !== 'string') {
+  for (var i = 0; i < arguments.length; i++) {
-      throw new TypeError('Arguments to path.join must be strings');
+    var arg = arguments[i];
+    assertPath(arg);
+    if (arg) {
+      paths.push(arg);
     }
-    return p;
   }
 
-  var paths = Array.prototype.filter.call(arguments, f);
   var joined = paths.join('\\');
 
   // Make sure that the joined path doesn't start with two slashes, because
@@ -232,25 +263,10 @@ win32.relative = function(from, to) {
   var lowerFrom = from.toLowerCase();
   var lowerTo = to.toLowerCase();
 
-  function trim(arr) {
+  var toParts = trimArray(to.split('\\'));
-    var start = 0;
-    for (; start < arr.length; start++) {
-      if (arr[start] !== '') break;
-    }
 
-    var end = arr.length - 1;
+  var lowerFromParts = trimArray(lowerFrom.split('\\'));
-    for (; end >= 0; end--) {
+  var lowerToParts = trimArray(lowerTo.split('\\'));
-      if (arr[end] !== '') break;
-    }
-
-    if (start > end) return [];
-    return arr.slice(start, end + 1);
-  }
-
-  var toParts = trim(to.split('\\'));
-
-  var lowerFromParts = trim(lowerFrom.split('\\'));
-  var lowerToParts = trim(lowerTo.split('\\'));
 
   var length = Math.min(lowerFromParts.length, lowerToParts.length);
   var samePartsLength = length;
@@ -356,15 +372,13 @@ win32.format = function(pathObject) {
 
   var dir = pathObject.dir;
   var base = pathObject.base || '';
-  if (dir.slice(dir.length - 1, dir.length) === win32.sep) {
+  if (!dir) {
+    return base;
+  }
+  if (dir[dir.length - 1] === win32.sep) {
     return dir + base;
   }
-
+  return dir + win32.sep + base;
-  if (dir) {
-    return dir + win32.sep + base;
-  }
-
-  return base;
 };
 
 
@@ -377,7 +391,7 @@ win32.parse = function(pathString) {
   }
   return {
     root: allParts[0],
-    dir: allParts[0] + allParts[1].slice(0, allParts[1].length - 1),
+    dir: allParts[0] + allParts[1].slice(0, -1),
     base: allParts[2],
     ext: allParts[3],
     name: allParts[2].slice(0, allParts[2].length - allParts[3].length)
@@ -418,7 +432,7 @@ posix.resolve = function() {
     }
 
     resolvedPath = path + '/' + resolvedPath;
-    resolvedAbsolute = path.charAt(0) === '/';
+    resolvedAbsolute = path[0] === '/';
   }
 
   // At this point the path should be resolved to a full absolute path, but
@@ -437,7 +451,7 @@ posix.normalize = function(path) {
   assertPath(path);
 
   var isAbsolute = posix.isAbsolute(path),
-      trailingSlash = path.substr(-1) === '/';
+      trailingSlash = path && path[path.length - 1] === '/';
 
   // Normalize the path
   path = normalizeArray(path.split('/'), !isAbsolute).join('/');
@@ -455,7 +469,7 @@ posix.normalize = function(path) {
 // posix version
 posix.isAbsolute = function(path) {
   assertPath(path);
-  return path.charAt(0) === '/';
+  return !!path && path[0] === '/';
 };
 
 // posix version
@@ -487,23 +501,8 @@ posix.relative = function(from, to) {
   from = posix.resolve(from).substr(1);
   to = posix.resolve(to).substr(1);
 
-  function trim(arr) {
+  var fromParts = trimArray(from.split('/'));
-    var start = 0;
+  var toParts = trimArray(to.split('/'));
-    for (; start < arr.length; start++) {
-      if (arr[start] !== '') break;
-    }
-
-    var end = arr.length - 1;
-    for (; end >= 0; end--) {
-      if (arr[end] !== '') break;
-    }
-
-    if (start > end) return [];
-    return arr.slice(start, end + 1);
-  }
-
-  var fromParts = trim(from.split('/'));
-  var toParts = trim(to.split('/'));
 
   var length = Math.min(fromParts.length, toParts.length);
   var samePartsLength = length;
@@ -602,7 +601,7 @@ posix.parse = function(pathString) {
 
   return {
     root: allParts[0],
-    dir: allParts[0] + allParts[1].slice(0, allParts[1].length - 1),
+    dir: allParts[0] + allParts[1].slice(0, -1),
     base: allParts[2],
     ext: allParts[3],
     name: allParts[2].slice(0, allParts[2].length - allParts[3].length)

test/parallel/test-path-parse-format.js

@@ -15,7 +15,12 @@ var winPaths = [
   '\\\\server two\\shared folder\\file path.zip',
   '\\\\teela\\admin$\\system32',
   '\\\\?\\UNC\\server\\share'
+];
 
+var winSpecialCaseFormatTests = [
+  [{dir: 'some\\dir'}, 'some\\dir\\'],
+  [{base: 'index.html'}, 'index.html'],
+  [{}, '']
 ];
 
 var unixPaths = [
@@ -30,6 +35,12 @@ var unixPaths = [
   'C:\\foo'
 ];
 
+var unixSpecialCaseFormatTests = [
+  [{dir: 'some/dir'}, 'some/dir/'],
+  [{base: 'index.html'}, 'index.html'],
+  [{}, '']
+];
+
 var errors = [
   {method: 'parse', input: [null],
    message: /Path must be a string. Received null/},
@@ -57,10 +68,12 @@ var errors = [
    message: /'pathObject.root' must be a string or undefined, not number/},
 ];
 
-check(path.win32, winPaths);
+checkParseFormat(path.win32, winPaths);
-check(path.posix, unixPaths);
+checkParseFormat(path.posix, unixPaths);
 checkErrors(path.win32);
 checkErrors(path.posix);
+checkFormat(path.win32, winSpecialCaseFormatTests);
+checkFormat(path.posix, unixSpecialCaseFormatTests);
 
 function checkErrors(path) {
   errors.forEach(function(errorCase) {
@@ -79,8 +92,7 @@ function checkErrors(path) {
   });
 }
 
-
+function checkParseFormat(path, paths) {
-function check(path, paths) {
   paths.forEach(function(element, index, array) {
     var output = path.parse(element);
     assert.strictEqual(path.format(output), element);
@@ -89,3 +101,9 @@ function check(path, paths) {
     assert.strictEqual(output.ext, path.extname(element));
   });
 }
+
+function checkFormat(path, testCases) {
+  testCases.forEach(function(testCase) {
+    assert.strictEqual(path.format(testCase[0]), testCase[1]);
+  });
+}