1
\$\begingroup\$

I was recently at a hackathon and saw quite a few people using Parse, so I decided to check it out and read some guides online. Can someone take a look at my code? I followed Parse documentation so it should be fairly readable. I'm trying to get the balance for the user when they log in and display it.

Parse.User.logIn("user", "pass", {
  success: function(user) {
    query.find({
      success: function(results) {
        results[0].save({ key: value }, {
          success: function(result) {

            var arr = result.balances;
            var balance = sumArray(arr);
            var charge = result.charge;

            if(balance < charge)
                document.write("Insufficient funds. Must have " + balance + charge + " balance left");
            else if(balance == charge)
                document.write("Oops!");
                document.write("Sorry, balance cannot reach zero");

            var mssg = "";
            var today = formatDate(new Date());

            for(var i=0; i<arr.length; i++) {
                mssg += arr[i] + " ,";
            }
            document.write(today - 1 + 1);
            document.write(mssg);

          },
          error: function(result, error) {
            console.log(error);
          }
        });
      },
      error: function(error) {
        console.log(error);
      }
    });
  },
  error: function(user, error) {
    console.log(error);
  }
});


function sumArray(arr) {
    var S = 0;
    for(var i=0; i<arr.length; i++){
        S += arr[i];
    }
    return S;
}

$(document).ready(function () {
    $("#dialog-confirm").dialog({
        autoOpen: false,
        height: 'auto',
        width: '400px',
        modal: true,
        buttons: {
            Cancel: function () {
                $(this).dialog("close");
            }
        }
    });

    $(function () {
        $("#form").datepicker({
            maxDate: '-18y'
        });
        $("#form").mask("99/99/9999", { placeholder: "mm/dd/yyyy" });
    })
});


function formatDate(date) {
    var d = new Date(date);
    var day = '' + d.getDate();

    return day;
}
\$\endgroup\$
0
1
\$\begingroup\$

Promises

Promises are the next big thing in JavaScript. You can read more about them here. Luckily, Parse supports Promises and you should make good use of them. Essentially, Promises will replace the messy callback pyramid structure you have here

Parse.User.logIn("user", "pass", {
  success: function(user) {
    ...
      success: function(results) {
        ...
          success: function(result) {
            ...
          },
          error: function(result, error) {
            // error
          }
        });
      },
      error: function(error) {
        // error
      }
    });
  },
  error: function(user, error) {
    // error
  }
});

Promises use the then function, which can be given callbacks to be called when the promise is fulfilled or has failed. Promises can also propogate errors, not calling any callbacks until an error handler is encountered. So, your code can be re-written as:

Parse.User.logIn("user", "pass").then(function(user) {
  return query.find();
}).then(function(results) {
  // ...
}).then(function(result) {
  // ...
}, function(error) {
  // error
});

One thing to remember that a successful promise does not imply the Parse Query itself was successful. A successful promise simply means your query was successfully executed, regardless of what value was returned by the query. So, make sure to check the return value from Parse:

.then(function(result)){   // Promise succeeded
  if(!result){             // check if return is not null
    // do something
  }
})


Duplicated event handling

You have this

$(document).ready(function () {

and then you also have this

$(function () {

Why? Just put all the content inside the .ready function and you are good to go


Beware of Javascript string concatenation

This is one of the most naive mistakes:

document.write("Insufficient funds. Must have " + balance + charge + " balance left");

Let's assume balance=10 and charge=20. You expect it to print :

Insufficient funds. Must have 30 balance left

What it will actually print is:

Insufficient funds. Must have 1020 balance left

String concatenation in javascript is not well-defined when types get mixed. In your case string + int types are getting mixed. What you need to do instead is:

document.write("Insufficient funds. Must have " + (balance + charge) + " balance left");

Notice the parantheses besides the int types.


Mixing logic and view

You are mixing your logic. Look here

document.write(mssg);

Code that is rendering the view such as document.write should be handled in a separate function. It becomes very messy and hard to debug when you combine your logic-side and view-side code in one place. Read up on MVC (Model-View-Controller).


Illusion of binding

Take a look here why dropping brackets and misleading indentation can have unforeseen consequences

if(balance < charge)
  document.write("Insufficient funds. Must have " + b + ch + " balance left");
else if(balance  == charge)
  document.write("Oops!");
  document.write("Sorry, balance cannot reach zero");

At what times do you think "Sorry, balance cannot reach zero" message will be printed? Will it be printed only when balance == charge? Unfortunately, no! It will be printed every single time!

Why? Because your indentation leads to you believe that the 2nd document.write statement is bound to the else clause, when infact it isn't. Dropping brackets might make code look more clean & readable, but make sure you don't let the indentation give you false illusion of binding.


Avoid unnecessary string creation

In Javascript, strings are immutable. When you do something like this

var mssg = "";
for(var i=0; i<arr.length; i++) {
    mssg += arr[i] + " ,";
}

it isn't "modifying" the mssg string, but instead creating a new string on every iteration. For large iterations, say a million, you will definitely have a performance hit. Other languages like C# solve this problem by using a StringBuilder class, but for Javascript you can use the join() method instead

var mssg = arr.join(", ");


Incorrect code

document.write(today - 1 + 1);

Why are you doing this? What is the purpose of -1 + 1? To me, it seems like you are trying to convert the date to an int. But you wouldn't need to do that if you hadn't done this earlier

var day = '' + d.getDate();

Don't add the empty string to the date in the first place. Just leave it as this

var day = d.getDate();


Underscore.js

Naive summation operations such as

function sumArray(arr) {
    var S = 0;
    for(var i=0; i<arr.length; a++){
        S += arr[i];
    }
    return S;
}

can be replaced by powerful helper functions provided by the Underscore.js library. Using reduce() function, we can re-write it as follows:

// return sum of all elements of an array
var sum = _.reduce(arr, function(memo, num){ return memo+ num; }, 0);
return sum;


Lacking comments

Please always comment your code.

\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.