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.
-
\$\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– Graipher2017年09月29日 15:07:17 +00:00Commented Sep 29, 2017 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– Michael2017年09月29日 16:34:25 +00:00Commented Sep 29, 2017 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– Mast ♦2017年09月29日 22:28:51 +00:00Commented Sep 29, 2017 at 22:28
1 Answer 1
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.