Problem
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.
Solution
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.