lib,test: minor refactoring

This refactors a couple tests to have upper case first characters in comments and to use `input` instead of `i`. It also adds a few TODOs and rewrites a few lines to use default arguments and to prevent function recreation when unnecessary. PR-URL: #19445 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
nodejs · Mar 25, 2018 · c1278e5 · c1278e5
1 parent c6b6c92
commit c1278e5
Show file tree

Hide file tree

Showing 16 changed files with 94 additions and 161 deletions.
diff --git a/lib/fs.js b/lib/fs.js
@@ -751,6 +751,10 @@ fs.ftruncate = function(fd, len = 0, callback) {
     len = 0;
   }
   validateUint32(fd, 'fd');
+  // TODO(BridgeAR): This does not seem right.
+  // There does not seem to be any validation before and if there is any, it
+  // should work similar to validateUint32 or not have a upper cap at all.
+  // This applies to all usage of `validateLen`.
   validateLen(len);
   len = Math.max(0, len);
   const req = new FSReqWrap();
@@ -1012,7 +1016,7 @@ fs.fchmod = function(fd, mode, callback) {
   mode = modeNum(mode);
   validateUint32(fd, 'fd');
   validateUint32(mode, 'mode');
-  // values for mode < 0 are already checked via the validateUint32 function
+  // Values for mode < 0 are already checked via the validateUint32 function
   if (mode > 0o777)
     throw new ERR_OUT_OF_RANGE('mode');
 
@@ -1025,7 +1029,8 @@ fs.fchmodSync = function(fd, mode) {
   mode = modeNum(mode);
   validateUint32(fd, 'fd');
   validateUint32(mode, 'mode');
-  if (mode < 0 || mode > 0o777)
+  // Values for mode < 0 are already checked via the validateUint32 function
+  if (mode > 0o777)
     throw new ERR_OUT_OF_RANGE('mode');
   const ctx = {};
   binding.fchmod(fd, mode, undefined, ctx);

diff --git a/lib/fs/promises.js b/lib/fs/promises.js
@@ -369,7 +369,7 @@ async function fchmod(handle, mode) {
   mode = modeNum(mode);
   validateFileHandle(handle);
   validateUint32(mode, 'mode');
-  if (mode < 0 || mode > 0o777)
+  if (mode > 0o777)
     throw new ERR_OUT_OF_RANGE('mode');
   return binding.fchmod(handle.fd, mode, kUsePromises);
 }

diff --git a/lib/internal/crypto/diffiehellman.js b/lib/internal/crypto/diffiehellman.js
@@ -39,13 +39,11 @@ function DiffieHellman(sizeOrKey, keyEncoding, generator, genEncoding) {
     );
   }
 
-  if (keyEncoding) {
-    if (typeof keyEncoding !== 'string' ||
-        (!Buffer.isEncoding(keyEncoding) && keyEncoding !== 'buffer')) {
-      genEncoding = generator;
-      generator = keyEncoding;
-      keyEncoding = false;
-    }
+  if (keyEncoding && !Buffer.isEncoding(keyEncoding) &&
+      keyEncoding !== 'buffer') {
+    genEncoding = generator;
+    generator = keyEncoding;
+    keyEncoding = false;
   }
 
   const encoding = getDefaultEncoding();

diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -752,7 +752,7 @@ E('ERR_INVALID_ARG_VALUE', (name, value, reason = 'is invalid') => {
     inspected = inspected.slice(0, 128) + '...';
   }
   return `The argument '${name}' ${reason}. Received ${inspected}`;
-}, TypeError, RangeError); // Some are currently falsy implemented as "Error"
+}, TypeError, RangeError);
 E('ERR_INVALID_ARRAY_LENGTH',
   (name, len, actual) => {
     internalAssert(typeof actual === 'number', 'actual must be a number');
@@ -762,7 +762,7 @@ E('ERR_INVALID_ASYNC_ID', 'Invalid %s value: %s', RangeError);
 E('ERR_INVALID_BUFFER_SIZE',
   'Buffer size must be a multiple of %s', RangeError);
 E('ERR_INVALID_CALLBACK', 'Callback must be a function', TypeError);
-E('ERR_INVALID_CHAR', invalidChar, TypeError); //Check falsy "Error" entries.
+E('ERR_INVALID_CHAR', invalidChar, TypeError);
 
 // This should probably be a `TypeError`.
 E('ERR_INVALID_CURSOR_POS',

diff --git a/lib/internal/fs.js b/lib/internal/fs.js
@@ -372,13 +372,9 @@ function validateOffsetLengthWrite(offset, length, byteLength) {
   }
 }
 
-function validatePath(path, propName) {
+function validatePath(path, propName = 'path') {
   let err;
 
-  if (propName === undefined) {
-    propName = 'path';
-  }
-
   if (typeof path !== 'string' && !isUint8Array(path)) {
     err = new ERR_INVALID_ARG_TYPE(propName, ['string', 'Buffer', 'URL'], path);
   } else {

diff --git a/lib/internal/process.js b/lib/internal/process.js
@@ -68,15 +68,15 @@ function setup_cpuUsage() {
       user: cpuValues[0],
       system: cpuValues[1]
     };
-
-    // Ensure that a previously passed in value is valid. Currently, the native
-    // implementation always returns numbers <= Number.MAX_SAFE_INTEGER.
-    function previousValueIsValid(num) {
-      return Number.isFinite(num) &&
-          num <= Number.MAX_SAFE_INTEGER &&
-          num >= 0;
-    }
   };
+
+  // Ensure that a previously passed in value is valid. Currently, the native
+  // implementation always returns numbers <= Number.MAX_SAFE_INTEGER.
+  function previousValueIsValid(num) {
+    return Number.isFinite(num) &&
+        num <= Number.MAX_SAFE_INTEGER &&
+        num >= 0;
+  }
 }
 
 // The 3 entries filled in by the original process.hrtime contains

diff --git a/lib/zlib.js b/lib/zlib.js
@@ -169,12 +169,16 @@ function flushCallback(level, strategy, callback) {
 // 4. Throws ERR_OUT_OF_RANGE for infinite numbers
 function checkFiniteNumber(number, name) {
   // Common case
-  if (number === undefined || Number.isNaN(number)) {
+  if (number === undefined) {
     return false;
   }
 
   if (Number.isFinite(number)) {
-    return true;  // is a valid number
+    return true; // Is a valid number
+  }
+
+  if (Number.isNaN(number)) {
+    return false;
   }
 
   // Other non-numbers

diff --git a/test/parallel/test-crypto-random.js b/test/parallel/test-crypto-random.js
@@ -80,43 +80,18 @@ process.setMaxListeners(256);
 }
 
 {
-  const buf = new Uint16Array(10);
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFillSync(buf);
-  const after = Buffer.from(buf.buffer).toString('hex');
-  assert.notStrictEqual(before, after);
-}
-
-{
-  const buf = new Uint32Array(10);
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFillSync(buf);
-  const after = Buffer.from(buf.buffer).toString('hex');
-  assert.notStrictEqual(before, after);
-}
-
-{
-  const buf = new Float32Array(10);
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFillSync(buf);
-  const after = Buffer.from(buf.buffer).toString('hex');
-  assert.notStrictEqual(before, after);
-}
-
-{
-  const buf = new Float64Array(10);
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFillSync(buf);
-  const after = Buffer.from(buf.buffer).toString('hex');
-  assert.notStrictEqual(before, after);
-}
-
-{
-  const buf = new DataView(new ArrayBuffer(10));
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFillSync(buf);
-  const after = Buffer.from(buf.buffer).toString('hex');
-  assert.notStrictEqual(before, after);
+  [
+    new Uint16Array(10),
+    new Uint32Array(10),
+    new Float32Array(10),
+    new Float64Array(10),
+    new DataView(new ArrayBuffer(10))
+  ].forEach((buf) => {
+    const before = Buffer.from(buf.buffer).toString('hex');
+    crypto.randomFillSync(buf);
+    const after = Buffer.from(buf.buffer).toString('hex');
+    assert.notStrictEqual(before, after);
+  });
 }
 
 {
@@ -140,53 +115,20 @@ process.setMaxListeners(256);
 }
 
 {
-  const buf = new Uint16Array(10);
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFill(buf, common.mustCall((err, buf) => {
-    assert.ifError(err);
-    const after = Buffer.from(buf.buffer).toString('hex');
-    assert.notStrictEqual(before, after);
-  }));
-}
-
-{
-  const buf = new Uint32Array(10);
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFill(buf, common.mustCall((err, buf) => {
-    assert.ifError(err);
-    const after = Buffer.from(buf.buffer).toString('hex');
-    assert.notStrictEqual(before, after);
-  }));
-}
-
-{
-  const buf = new Float32Array(10);
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFill(buf, common.mustCall((err, buf) => {
-    assert.ifError(err);
-    const after = Buffer.from(buf.buffer).toString('hex');
-    assert.notStrictEqual(before, after);
-  }));
-}
-
-{
-  const buf = new Float64Array(10);
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFill(buf, common.mustCall((err, buf) => {
-    assert.ifError(err);
-    const after = Buffer.from(buf.buffer).toString('hex');
-    assert.notStrictEqual(before, after);
-  }));
-}
-
-{
-  const buf = new DataView(new ArrayBuffer(10));
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFill(buf, common.mustCall((err, buf) => {
-    assert.ifError(err);
-    const after = Buffer.from(buf.buffer).toString('hex');
-    assert.notStrictEqual(before, after);
-  }));
+  [
+    new Uint16Array(10),
+    new Uint32Array(10),
+    new Float32Array(10),
+    new Float64Array(10),
+    new DataView(new ArrayBuffer(10))
+  ].forEach((buf) => {
+    const before = Buffer.from(buf.buffer).toString('hex');
+    crypto.randomFill(buf, common.mustCall((err, buf) => {
+      assert.ifError(err);
+      const after = Buffer.from(buf.buffer).toString('hex');
+      assert.notStrictEqual(before, after);
+    }));
+  });
 }
 
 {

diff --git a/test/parallel/test-fs-access.js b/test/parallel/test-fs-access.js
@@ -62,24 +62,16 @@ assert.strictEqual(typeof fs.R_OK, 'number');
 assert.strictEqual(typeof fs.W_OK, 'number');
 assert.strictEqual(typeof fs.X_OK, 'number');
 
-fs.access(__filename, common.mustCall((err) => {
-  assert.ifError(err);
-}));
-
-fs.access(__filename, fs.R_OK, common.mustCall((err) => {
-  assert.ifError(err);
-}));
+fs.access(__filename, common.mustCall(assert.ifError));
+fs.access(__filename, fs.R_OK, common.mustCall(assert.ifError));
+fs.access(readOnlyFile, fs.F_OK | fs.R_OK, common.mustCall(assert.ifError));
 
 fs.access(doesNotExist, common.mustCall((err) => {
   assert.notStrictEqual(err, null, 'error should exist');
   assert.strictEqual(err.code, 'ENOENT');
   assert.strictEqual(err.path, doesNotExist);
 }));
 
-fs.access(readOnlyFile, fs.F_OK | fs.R_OK, common.mustCall((err) => {
-  assert.ifError(err);
-}));
-
 fs.access(readOnlyFile, fs.W_OK, common.mustCall(function(err) {
   assert.strictEqual(this, undefined);
   if (hasWriteAccessForReadonlyFile) {

diff --git a/test/parallel/test-fs-truncate.js b/test/parallel/test-fs-truncate.js
@@ -36,7 +36,7 @@ let stat;
 const msg = 'Using fs.truncate with a file descriptor is deprecated.' +
             ' Please use fs.ftruncate with a file descriptor instead.';
 
-// truncateSync
+// Check truncateSync
 fs.writeFileSync(filename, data);
 stat = fs.statSync(filename);
 assert.strictEqual(stat.size, 1024 * 16);
@@ -49,7 +49,7 @@ fs.truncateSync(filename);
 stat = fs.statSync(filename);
 assert.strictEqual(stat.size, 0);
 
-// ftruncateSync
+// Check ftruncateSync
 fs.writeFileSync(filename, data);
 const fd = fs.openSync(filename, 'r+');
 
@@ -64,18 +64,16 @@ fs.ftruncateSync(fd);
 stat = fs.statSync(filename);
 assert.strictEqual(stat.size, 0);
 
-// truncateSync
+// Check truncateSync
 common.expectWarning('DeprecationWarning', msg);
 fs.truncateSync(fd);
 
 fs.closeSync(fd);
 
-// async tests
+// Async tests
 testTruncate(common.mustCall(function(er) {
   assert.ifError(er);
-  testFtruncate(common.mustCall(function(er) {
-    assert.ifError(er);
-  }));
+  testFtruncate(common.mustCall(assert.ifError));
 }));
 
 function testTruncate(cb) {
@@ -105,7 +103,6 @@ function testTruncate(cb) {
   });
 }
 
-
 function testFtruncate(cb) {
   fs.writeFile(filename, data, function(er) {
     if (er) return cb(er);
@@ -136,7 +133,6 @@ function testFtruncate(cb) {
   });
 }
 
-
 // Make sure if the size of the file is smaller than the length then it is
 // filled with zeroes.