CODEWITHSUNDEEP
javascriptnode.jsmongodbexpress
Shingo Wu
Shingo Wu

Reputation: 33

How to make my code clearer

The code is for handling the POST request within Expressjs and mongodb

router.post('/', function(req, res){
  var data = req.body;
  Tag.find({name: data.name}).limit(1).exec( function(err, result){
    if(err){
    } else {
      if(result.length > 0){ // Already exist a tag with same name
        res.status(400).end('Already exist!');
      } else { // Save the new Tag to database
        var tag = new Tag();
        tag.name = data.name;
        tag.lastModifier = req.user?req.user.username:"system";
        tag.lastModified = Date.now();
        tag.save(function(err){
          if(err){
            res.status(400).json({ 
              message: "insert tag error"
            });
          } else {
            Tag.findOne(tag, function(err, result){
              if(err){          
                res.status(400).json({
                  message: "some error.."
                });
              } else {
                //res.status(400).end('same tag name');
                res.status(201).json({
                  _id: result._id
                });          
              }
            });
          }
        });
      }
    }
  });
});

The stairs in the last 9 lines are terrible....please teach me how could I make this mess clearer?

Upvotes: 1

Views: 68

Answers (4)

mudonat
mudonat

Reputation: 36

router.post('/', function(req, res){
  var data = req.body;
  Tag.find({name: data.name}).limit(1).exec(cbExec);
});


function cbExec(err, result){
    if(err){
    } else {
        if(result.length > 0){ // Already exist a tag with same name
            res.status(400).end('Already exist!');
        } else { // Save the new Tag to database
            var tag = new Tag();
            tag.name = data.name;
            tag.lastModifier = req.user?req.user.username:"system";
            tag.lastModified = Date.now();
            tag.save(cbSave);
        }
    }
}

function cbSave(err){
    if(err){
        res.status(400).json({message: "insert tag error"});
    } else {
        Tag.findOne(tag, cbTag);
    }
}

function cbTag(err, result){
    if(err){        
        res.status(400).json({message: "some error.."});
    } else {
        //res.status(400).end('same tag name');
        res.status(201).json({_id: result._id});          
    }
}

Upvotes: 1

Guffa
Guffa

Reputation: 700372

You can use named functions instead of some of the function expressions:

router.post('/', function(req, res){
  var data = req.body;
  Tag.find({name: data.name}).limit(1).exec( function(err, result){
    if(err){
    } else {
      if(result.length > 0){ // Already exist a tag with same name
        res.status(400).end('Already exist!');
      } else { // Save the new Tag to database
        var tag = new Tag();
        tag.name = data.name;
        tag.lastModifier = req.user?req.user.username:"system";
        tag.lastModified = Date.now();
        tag.save(save(err));
      }
    }
  });
});

function save(err){
  if(err){
    res.status(400).json({ 
      message: "insert tag error"
    });
  } else {
    Tag.findOne(tag, handleResult(err, result));
  }
}

function handleResult(err, result){
  if(err){          
    res.status(400).json({
      message: "some error.."
    });
  } else {
    //res.status(400).end('same tag name');
    res.status(201).json({
      _id: result._id
    });          
  }
}

(You can surely name them a little more appropriate for the situation, but it shows the principle.)

Upvotes: 1

krzysiej
krzysiej

Reputation: 898

You can separate the cod a little bi more. Instead of creating lambda functions create normal ones. You can get rid of one pair of braces in 4th line if(err){ } else {

using if(!err)

Upvotes: 0

Jakub Arnold
Jakub Arnold

Reputation: 87220

I really recommend you to try promises. There are many implementations available for JavaScript and Node.js.

A promise basically encapsulates an asynchronous operation into a value, which allows you to get rid of these horrible nested callbacks. They also allow you to chain asynchronous operations more easily.

What you're forced to do in your callback-based code is to check errors at every level, which can get rather tedious if your error handling could be at one place. Promises will propagate the error, allowing easy handling in one place.

Here are some references:

It might take a little while to adjust to using them, but trust me, it is absolutely worth it.

Upvotes: 0

Related Questions