1
\$\begingroup\$

I have this code that generates a utilization report for a company I am interning for that uses a time-tracking app called Harvest.

var config = require('./config');
var request = require('request');
var moment = require('moment');
var pageNumber, pages;
var userArray = [];
var iteration = 0;
var pageNumber = 1;

let reqURL = 'https://api.harvestapp.com/v2/users?page=1&per_page=100';
let reqURL2 = 'https://api.harvestapp.com/v2/time_entries?from=' + new moment().day(-1).format('YYYY-MM-DD') + '&to=' + new moment().day(5).format('YYYY-MM-DD') + '&page=1&per_page=100';

var options = {
    url: reqURL,
    headers: {
      'Authorization': 'Bearer' + ' ' + config.token,
      'Harvest-Account-Id': config.accountID,
      'User-Agent': config.userAgent
    }
  };

  var options2 = {
    url: reqURL2,
    headers: {
      'Authorization': 'Bearer' + ' ' + config.token,
      'Harvest-Account-Id': config.accountID,
      'User-Agent': config.userAgent
    }
  }

  function callback1(error, response, body) {

    if (!error && response.statusCode == 200) {
      info = JSON.parse(body);
      pages = info.total_pages;

      for(x = 0; x < info.users.length; x++) {
        userArray[iteration] = new Object(),
        userArray[iteration].id = info.users[x].id,
        userArray[iteration].name = info.users[x].first_name + ' ' + info.users[x].last_name
        userArray[iteration].billableHours = 0;
        userArray[iteration].totalHours = 0;
        iteration++;
      };

      if(pageNumber < info.total_pages) {
        pageNumber++;
        options.url = 'https://api.harvestapp.com/v2/users?page=' + pageNumber + '&per_page=100';
        request(options, callback1);
      } else {
        pageNumber = 1;
      }

    };

  };

  request(options, callback1);

  function callback2(error, response, body) {
    if (!error && response.statusCode == 200) {

      info = JSON.parse(body);
      pages = info.total_pages;

      for(y = 0; y < info.time_entries.length; y++){

        for(x = 0; x < userArray.length; x++) {

          if(info.time_entries[y].user.id == userArray[x].id) {
            userArray[x].totalHours += info.time_entries[y].hours;

            if(info.time_entries[y].billable) {
              userArray[x].billableHours += info.time_entries[y].hours;
            }
            break;

          }

    }

  }
  if(pageNumber < info.total_pages) {
    pageNumber++;
    options2.url = 'https://api.harvestapp.com/v2/time_entries?from=' + new moment().day(-1).format('YYYY-MM-DD') + '&to=' + new moment().day(5).format('YYYY-MM-DD') + '&page=' + pageNumber + '&per_page=100';
    request(options2, callback2);
  } else {
    removeUsers(userArray, 'totalHours', 0);
  }

}

 }

 request(options2, callback2);

 function removeUsers(array, attribute, value) {
 var i = array.length;

while(i--) {
  array[i].utilization = Math.ceil(100*(array[i].billableHours/40));

  if(array[i] && array[i].hasOwnProperty(attribute) && (arguments.length > 2 && array[i][attribute] === value)){
    array.splice(i,1);
  }

}
console.log(array);
}

I'm sorry about the spacing down here, for some reason it wouldn't recognize it as code. If anyone has any optimization ideas those would be much appreciated.

\$\endgroup\$
  • \$\begingroup\$ Hey, welcome to Code Review! This question does not match what this site is about. Code Review is about improving existing, working code. Code Review is not the site to ask for help in fixing or changing what your code does. Once the code does what you want, we would love to help you do the same thing in a cleaner way! Please see our help center for more information. \$\endgroup\$ – Graipher Sep 29 '17 at 15:07
  • \$\begingroup\$ Took out the portion that asks for help on the bug fixing, now it is purely optimization as it works for what I need it for right now, that bug was just a side-thought that I thought I'd ask about while I was here. Sorry! \$\endgroup\$ – Michael Sep 29 '17 at 16:34
  • \$\begingroup\$ No no, don't hide the bug. Tell us of the bug and tell us you can live with it for now. All of a sudden it works as intended and we'll see what we can do. \$\endgroup\$ – Mast Sep 29 '17 at 22:28
1
\$\begingroup\$

Appending to an array

The global iteration variable is used as the index to set objects in the global userArray. This implementation practically appends items to the array:

userArray[iteration] = new Object(),
userArray[iteration].id = info.users[x].id,
userArray[iteration].name = info.users[x].first_name + ' ' + info.users[x].last_name
userArray[iteration].billableHours = 0;
userArray[iteration].totalHours = 0;
iteration++;

A simpler alternative is to use the push method of arrays instead of the iteration variable. Also, instead of setting the fields one by one, it will be shorter to use an object literal:

userArray.push({
  id: info.users[x].id,
  name: info.users[x].first_name + ' ' + info.users[x].last_name,
  billableHours: 0,
  totalHours: 0
});

Data structures

In callback2, for each time entry, you loop over userArray until you find a user with the matching id. You could avoid this nested loop by using a mapping of user ids to users. If you don't need to preserve the ordering of users, then you can drop userArray in favor of the map. If you need to preserve the ordering, you can keep both.

Punctuation

The punctuation here is messy and potentially buggy:

userArray[iteration] = new Object(),
userArray[iteration].id = info.users[x].id,
userArray[iteration].name = info.users[x].first_name + ' ' + info.users[x].last_name
userArray[iteration].billableHours = 0;
userArray[iteration].totalHours = 0;
iteration++;

Each line should have ended with a ;, instead of , or nothing.

Don't repeat yourself

The options and options2 variables are almost identical. Try to avoid copy-pasting, use helper methods or benefit from JavaScript's prototypal inheritance instead.

Naming

Instead of numbering in names such as options1, callback1, and others, it would be better to use more meaningful names.

\$\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.