Merge pull request #3478 from muxator/flatten-code

This series is an attempt to reduce the control structure depth of the code
base, maintaining at the same time its exact same behaviour, bugs included. It
is, in a sense, an initial attempt at a refactoring in the spirit of its
original definition [0].

The idea beyond this refactoring is that reducing the code depth and, sometimes,
inverting some conditions, bugs and logic errors may become easier to spot, and
the code easier to read.

When looked at while ignoring white space changes, all of these diffs should
appear trivial.

[0] https://refactoring.com/

src/node/db/API.js

@@ -137,15 +137,13 @@ exports.getRevisionChangeset = function(padID, rev, callback)
   if (rev !== undefined && typeof rev !== "number")
   {
     // try to parse the number
-    if (!isNaN(parseInt(rev)))
+    if (isNaN(parseInt(rev)))
-    {
-      rev = parseInt(rev);
-    }
-    else
     {
       callback(new customError("rev is not a number", "apierror"));
       return;
     }
+
+    rev = parseInt(rev);
   }
 
   // ensure this is not a negative number
@@ -184,17 +182,17 @@ exports.getRevisionChangeset = function(padID, rev, callback)
         
         callback(null, changeset);
       })
+
+      return;
     }
+
     //the client wants the latest changeset, lets return it to him
-    else
-    {
     pad.getRevisionChangeset(pad.getHeadRevisionNumber(), function(err, changeset)
     {
       if(ERR(err, callback)) return;
 
       callback(null, changeset);
     })
-    }
   });
 }
 
@@ -219,15 +217,13 @@ exports.getText = function(padID, rev, callback)
   if(rev !== undefined && typeof rev != "number")
   {
     //try to parse the number
-    if(!isNaN(parseInt(rev)))
+    if(isNaN(parseInt(rev)))
-    {
-      rev = parseInt(rev);
-    }
-    else
     {
       callback(new customError("rev is not a number", "apierror"));
       return;
     }
+
+    rev = parseInt(rev);
   }
   
   //ensure this is not a negativ number
@@ -268,13 +264,13 @@ exports.getText = function(padID, rev, callback)
         
         callback(null, data);
       })
+
+      return;
     }
+
     //the client wants the latest text, lets return it to him
-    else
-    {
     var padText = exportTxt.getTXTFromAtext(pad, pad.atext);
     callback(null, {"text": padText});
-    }
   });
 }
 
@@ -359,15 +355,13 @@ exports.getHTML = function(padID, rev, callback)
 
   if (rev !== undefined && typeof rev != "number")
   {
-    if (!isNaN(parseInt(rev)))
+    if (isNaN(parseInt(rev)))
-    {
-      rev = parseInt(rev);
-    }
-    else
     {
       callback(new customError("rev is not a number","apierror"));
       return;
     }
+
+    rev = parseInt(rev);
   }
 
   if(rev !== undefined && rev < 0)
@@ -405,10 +399,11 @@ exports.getHTML = function(padID, rev, callback)
           var data = {html: html};
           callback(null, data);
       });
+
+      return;
     }
+
     //the client wants the latest text, lets return it to him
-    else
-    {
     exportHtml.getPadHTML(pad, undefined, function (err, html)
     {
       if(ERR(err, callback)) return;
@@ -417,7 +412,6 @@ exports.getHTML = function(padID, rev, callback)
       var data = {html: html};
       callback(null, data);
     });
-    }
   });
 }
 
@@ -448,11 +442,10 @@ exports.setHTML = function(padID, html, callback)
       if(e){
         callback(new customError("HTML is malformed","apierror"));
         return;
-      }else{
+      }
+
       //update the clients on the pad
       padMessageHandler.updatePadClients(pad, callback);
-        return;
-      }
     });
   });
 }
@@ -641,15 +634,13 @@ exports.saveRevision = function(padID, rev, callback)
   if(rev !== undefined && typeof rev != "number")
   {
     //try to parse the number
-    if(!isNaN(parseInt(rev)))
+    if(isNaN(parseInt(rev)))
-    {
-      rev = parseInt(rev);
-    }
-    else
     {
       callback(new customError("rev is not a number", "apierror"));
       return;
     }
+
+    rev = parseInt(rev);
   }
 
   //ensure this is not a negativ number
@@ -732,8 +723,9 @@ exports.createPad = function(padID, text, callback)
       callback(new customError("createPad can't create group pads","apierror"));
       return;
     }
+
     //check for url special characters
-    else if(padID.match(/(\/|\?|&|#)/))
+    if(padID.match(/(\/|\?|&|#)/))
     {
       callback(new customError("malformed padID: Remove special characters","apierror"));
       return;
@@ -782,15 +774,13 @@ exports.restoreRevision = function (padID, rev, callback)
   if (rev !== undefined && typeof rev != "number")
   {
     //try to parse the number
-    if (!isNaN(parseInt(rev)))
+    if (isNaN(parseInt(rev)))
-    {
-      rev = parseInt(rev);
-    }
-    else
     {
       callback(new customError("rev is not a number", "apierror"));
       return;
     }
+
+    rev = parseInt(rev);
   }
 
   //ensure this is not a negativ number
@@ -959,11 +949,10 @@ exports.getPadID = function(roID, callback)
     if(retrievedPadID == null)
     {
       callback(new customError("padID does not exist","apierror"));
+      return;
     }
-    else
+
-    {
     callback(null, {padID: retrievedPadID});
-    }
   });
 }
 
@@ -1127,9 +1116,9 @@ exports.sendClientsMessage = function (padID, msg, callback) {
   getPadSafe(padID, true, function (err, pad) {
     if (ERR(err, callback)) {
       return;
-    } else {
-      padMessageHandler.handleCustomMessage(padID, msg, callback);
     }
+
+    padMessageHandler.handleCustomMessage(padID, msg, callback);
   } );
 }
 
@@ -1177,30 +1166,26 @@ exports.createDiffHTML = function(padID, startRev, endRev, callback){
   if(startRev !== undefined && typeof startRev != "number")
   {
     //try to parse the number
-    if(!isNaN(parseInt(startRev)))
+    if(isNaN(parseInt(startRev)))
-    {
-      startRev = parseInt(startRev, 10);
-    }
-    else
     {
       callback({stop: "startRev is not a number"});
       return;
     }
+
+    startRev = parseInt(startRev, 10);
   }
  
   //check if rev is a number
   if(endRev !== undefined && typeof endRev != "number")
   {
     //try to parse the number
-    if(!isNaN(parseInt(endRev)))
+    if(isNaN(parseInt(endRev)))
-    {
-      endRev = parseInt(endRev, 10);
-    }
-    else
     {
       callback({stop: "endRev is not a number"});
       return;
     }
+
+    endRev = parseInt(endRev, 10);
   }
  
   //get the pad

src/node/db/AuthorManager.js

@@ -104,16 +104,16 @@ function mapAuthorWithDBKey (mapperkey, mapper, callback)
         //return the author
         callback(null, author);
       });
+
+      return;
     }
+
     //there is a author with this mapper
-    else
-    {
     //update the timestamp of this author
     db.setSub("globalAuthor:" + author, ["timestamp"], new Date().getTime());
 
     //return the author
     callback(null, {authorID: author});
-    }
   });
 }
 
@@ -209,10 +209,10 @@ exports.listPadsOfAuthor = function (authorID, callback)
     if(author == null)
     {
       callback(new customError("authorID does not exist","apierror"))
+      return;
     }
+
     //everything is fine, return the pad IDs
-    else
-    {
     var pads = [];
     if(author.padIDs != null)
     {
@@ -222,7 +222,6 @@ exports.listPadsOfAuthor = function (authorID, callback)
       }
     }
     callback(null, {padIDs: pads});
-    }
   });
 }
 

src/node/db/GroupManager.js

@@ -62,13 +62,12 @@ exports.deleteGroup = function(groupID, callback)
         if(_group == null)
         {
           callback(new customError("groupID does not exist","apierror"));
+          return;
         }
+
         //group exists, everything is fine
-        else
-        {
         group = _group;
         callback();
-        }
       });
     },
     //iterate trough all pads of this groups and delete them
@@ -213,23 +212,6 @@ exports.createGroupIfNotExistsFor = function(groupMapper, callback)
   //try to get a group for this mapper
   db.get("mapper2group:"+groupMapper, function(err, groupID)
   {
-     if(ERR(err, callback)) return;
-     
-     // there is a group for this mapper
-     if(groupID) {
-       exports.doesGroupExist(groupID, function(err, exists) {
-         if(ERR(err, callback)) return;
-         if(exists) return callback(null, {groupID: groupID});
-         
-         // hah, the returned group doesn't exist, let's create one
-         createGroupForMapper(callback)
-       })
-     }
-     //there is no group for this mapper, let's create a group
-     else {
-       createGroupForMapper(callback)
-     }
-     
     function createGroupForMapper(cb) {
       exports.createGroup(function(err, responseObj)
       {
@@ -241,6 +223,24 @@ exports.createGroupIfNotExistsFor = function(groupMapper, callback)
         cb(null, responseObj);
       });
     }
+
+    if(ERR(err, callback)) return;
+     
+    // there is a group for this mapper
+    if(groupID) {
+      exports.doesGroupExist(groupID, function(err, exists) {
+        if(ERR(err, callback)) return;
+        if(exists) return callback(null, {groupID: groupID});
+
+        // hah, the returned group doesn't exist, let's create one
+        createGroupForMapper(callback)
+      })
+
+      return;
+    }
+
+    //there is no group for this mapper, let's create a group
+    createGroupForMapper(callback)
   });
 }
 
@@ -261,12 +261,11 @@ exports.createGroupPad = function(groupID, padName, text, callback)
         if(exists == false)
         {
           callback(new customError("groupID does not exist","apierror"));
+          return;
         }
+
         //group exists, everything is fine
-        else
-        {
         callback();
-        }
       });
     },
     //ensure pad does not exists
@@ -280,12 +279,11 @@ exports.createGroupPad = function(groupID, padName, text, callback)
         if(exists == true)
         {
           callback(new customError("padName does already exist","apierror"));
+          return;
         }
+
         //pad does not exist, everything is fine
-        else
-        {
         callback();
-        }
       });
     },
     //create the pad
@@ -320,10 +318,10 @@ exports.listPads = function(groupID, callback)
     if(exists == false)
     {
       callback(new customError("groupID does not exist","apierror"));
+      return;
     }
+
     //group exists, let's get the pads
-    else
-    {
     db.getSub("group:" + groupID, ["pads"], function(err, result)
     {
       if(ERR(err, callback)) return;
@@ -333,6 +331,5 @@ exports.listPads = function(groupID, callback)
       }
       callback(null, {padIDs: pads});
     });
-    }
   });
 }

src/node/db/Pad.js

@@ -476,8 +476,12 @@ Pad.prototype.copy = function copy(destinationID, force, callback) {
     // if it's a group pad, let's make sure the group exists.
     function(callback)
     {
-      if (destinationID.indexOf("$") != -1)
+      if (destinationID.indexOf("$") === -1)
       {
+        callback();
+        return;
+      }
+
       destGroupID = destinationID.split("$")[0]
       groupManager.doesGroupExist(destGroupID, function (err, exists)
       {
@@ -489,15 +493,10 @@ Pad.prototype.copy = function copy(destinationID, force, callback) {
           callback(new customError("groupID does not exist for destinationID","apierror"));
           return;
         }
+
         //everything is fine, continue
-          else
-          {
         callback();
-          }
       });
-      }
-      else
-        callback();
     },
     // if the pad exists, we should abort, unless forced.
     function(callback)
@@ -506,8 +505,16 @@ Pad.prototype.copy = function copy(destinationID, force, callback) {
       {
         if(ERR(err, callback)) return;
 
-        if(exists == true)
+        /*
+         * this is the negation of a truthy comparison. Has been left in this
+         * wonky state to keep the old (possibly buggy) behaviour
+         */
+        if (!(exists == true))
         {
+          callback();
+          return;
+        }
+
         if (!force)
         {
           console.error("erroring out without force");
@@ -515,18 +522,12 @@ Pad.prototype.copy = function copy(destinationID, force, callback) {
           console.error("erroring out without force - after");
           return;
         }
-          else // exists and forcing
+
-          {
+        // exists and forcing
         padManager.getPad(destinationID, function(err, pad) {
           if (ERR(err, callback)) return;
           pad.remove(callback);
         });
-          }
-        }
-        else
-        {
-          callback();
-        }
       });
     },
     // copy the 'pad' entry
@@ -622,9 +623,14 @@ Pad.prototype.remove = function remove(callback) {
         //is it a group pad? -> delete the entry of this pad in the group
         function(callback)
         {
-          //is it a group pad?
+          if(padID.indexOf("$") === -1)
-          if(padID.indexOf("$")!=-1)
           {
+            // it isn't a group pad, nothing to do here
+            callback();
+            return;
+          }
+
+          // it is a group pad
           var groupID = padID.substring(0,padID.indexOf("$"));
 
           db.get("group:" + groupID, function (err, group)
@@ -639,12 +645,6 @@ Pad.prototype.remove = function remove(callback) {
 
             callback();
           });
-          }
-          //its no group pad, nothing to do here
-          else
-          {
-            callback();
-          }
         },
         //remove the readonly entries
         function(callback)

src/node/db/PadManager.js

@@ -159,10 +159,10 @@ exports.getPad = function(id, text, callback)
   if(pad != null)
   {
     callback(null, pad);
+    return;
   }
+
   //try to load pad
-  else
-  {
   pad = new Pad(id);
 
   //initalize the pad
@@ -173,7 +173,6 @@ exports.getPad = function(id, text, callback)
     padList.addPad(id);
     callback(null, pad);
   });
-  }
 }
 
 exports.listAllPads = function(cb)
@@ -206,18 +205,18 @@ exports.sanitizePadId = function(padId, callback) {
   if(transform_index >= padIdTransforms.length)
   {
     callback(padId);
+    return;
   }
+
   //check if padId exists
-  else
-  {
   exports.doesPadExists(padId, function(junk, exists)
   {
     if(exists)
     {
       callback(padId);
+      return;
     }
-      else
+
-      {
     //get the next transformation *that's different*
     var transformedPadId = padId;
     while(transformedPadId == padId && transform_index < padIdTransforms.length)
@@ -227,9 +226,7 @@ exports.sanitizePadId = function(padId, callback) {
     }
     //check the next transform
     exports.sanitizePadId(transformedPadId, callback, transform_index);
-      }
   });
-  }
 }
 
 exports.isValidPadId = function(padId)

src/node/db/SecurityManager.js

@@ -90,13 +90,13 @@ exports.checkAccess = function (padID, sessionCookie, token, password, callback)
             // grant or deny access, with author of token
             callback(null, statusObject);
           });
+
+          return;
         }
+
         // user may create new pads - no need to check anything
-        else
-        {
         // grant access, with author of token
         callback(null, statusObject);
-        }
       });
       
       //don't continue

src/node/db/SessionManager.js

@@ -90,15 +90,13 @@ exports.createSession = function(groupID, authorID, validUntil, callback)
       if(typeof validUntil != "number")
       {
         //try to parse the number
-        if(!isNaN(parseInt(validUntil)))
+        if(isNaN(parseInt(validUntil)))
-        {
-          validUntil = parseInt(validUntil);
-        }
-        else
         {
           callback(new customError("validUntil is not a number","apierror"));
           return;
         }
+
+        validUntil = parseInt(validUntil);
       }
       
       //ensure this is not a negativ number

src/node/handler/PadMessageHandler.js

@@ -244,7 +244,19 @@ exports.handleMessage = function(client, message)
     }
   };
 
-  if (message) {
+  /*
+   * In a previous version of this code, an "if (message)" wrapped the
+   * following async.series().
+   * This ugly "!Boolean(message)" is a lame way to exactly negate the truthy
+   * condition and replace it with an early return, while being sure to leave
+   * the original behaviour unchanged.
+   *
+   * A shallower code could maybe make more evident latent logic errors.
+   */
+  if (!Boolean(message)) {
+    return;
+  }
+
   async.series([
     handleMessageHook,
     //check permissions
@@ -265,7 +277,8 @@ exports.handleMessage = function(client, message)
       if(!sessioninfos[client.id].auth){
         console.error("Auth was never applied to a session.  If you are using the stress-test tool then restart Etherpad and the Stress test tool.")
         return;
-        }else{
+      }
+
       var auth = sessioninfos[client.id].auth;
       var checkAccessCallback = function(err, statusObject)
       {
@@ -282,6 +295,7 @@ exports.handleMessage = function(client, message)
           client.json.send({accessStatus: statusObject.accessStatus})
         }
       };
+
       //check if pad is requested via readOnly
       if (auth.padID.indexOf("r.") === 0) {
         //Pad is readOnly, first get the real Pad ID
@@ -292,11 +306,9 @@ exports.handleMessage = function(client, message)
       } else {
         securityManager.checkAccess(auth.padID, auth.sessionID, auth.token, auth.password, checkAccessCallback);
       }
-        }
     },
     finalHandler
   ]);
-  }
 }
 
 

src/node/hooks/express/adminsettings.js

@@ -28,8 +28,7 @@ exports.socketio = function (hook_name, args, cb) {
         if (err) {
           return console.log(err);
         }
-        else
+
-        {
         //if showSettingsInAdminPage is set to false, then return NOT_ALLOWED in the result
         if(settings.showSettingsInAdminPage === false) {
           socket.emit("settings", {results:'NOT_ALLOWED'});
@@ -37,7 +36,6 @@ exports.socketio = function (hook_name, args, cb) {
         else {
           socket.emit("settings", {results: data});
         }
-        }
       });
     });
 

src/node/hooks/express/padurlsanitize.js

@@ -8,9 +8,9 @@ exports.expressCreateServer = function (hook_name, args, cb) {
     if(!padManager.isValidPadId(padId) || /\/$/.test(req.url))
     {
       res.status(404).send('Such a padname is forbidden');
+      return;
     }
-    else
+
-    {
     padManager.sanitizePadId(padId, function(sanitizedPadId) {
       //the pad id was sanitized, so we redirect to the sanitized version
       if(sanitizedPadId != padId)
@@ -28,6 +28,5 @@ exports.expressCreateServer = function (hook_name, args, cb) {
         next();
       }
     });
-    }
   });
 }