First, a few general pointers.
- Commented out code is bad. It tells readers that you don't really know why this code was there, but removing it seems to have fixed something, or maybe adding it broke something.
- When accessing object properties, avoid using
obj["prop"]
if possible. Just use obj.prop
.
- Try to choose meaningful names, naming things is one of the hardest things to do, but a descriptive name can go a long way towards making code more readable. I'll try to provide some advice on a case-by-case basis when going through the code.
Working through this bit by bit, I have next to no experience with jQuery so will only skim those parts. I wrote most of this before Vogel posted an answer, so there may be some duplication here.
First, getting epochSeconds
.
var currentDateTime = new Date();
var epochSeconds = Math.round(currentDateTime.getTime() / 1000);
There's no need to create a new Date()
to get the current epoch time, I would also prefer Math.floor
to Math.round
to avoid getting a future time (even though that future time is very close).
var epochSeconds = Math.floor(Date.now())
It is very easy to forget that this variable exists later on in the code, and it really isn't that expensive to recreate later on, so I would recommend either moving this into the function that needs it, or creating a function to get the current time if it is used in multiple functions. Declaring epochSeconds
up here also introduces a bug if it takes more than 1 second to fetch question/answer info as the time calculated from "now" may end up being negative.
As @Tamoghna Chowdhury said in the comments, your limit
function is already built in to the Math
object. You can use Math.max
and Math.min
to get the maximum and minimum values of the passed arguments. Another advantage to using Math.max
and Math.min
is that the code will be easier to read. If limit
is seen out of context, it could easily be assumed that it returns either the maximum or the minimum value.
Skipping down to zeroToOneHundredScore
, I wish I could suggest a clearer name as the name threw me off for a bit, but I can't seem to come up with anything that I would claim is better at representing the function's purpose without an accompanying comment. Personally, I would likely name this function grade
or score
and add a JSDoc comment that clarifies that it returns a number between 0 and 100.
I would personally slightly change how this method is written. First, check for the special cases. There is no need to calculate the ratio first as you know that if (number < min)
then 0
should be returned, and if (number > max)
then 100
should be returned. By checking for the special cases first, the method becomes easier for someone who has no idea what it does to grasp quickly.
function zeroToOneHundredScore(number, min, max) {
if (number < min) {
return 0;
}
if (number > max) {
return 100;
}
var lowerDifference = number - min;
var minnedMax = max - min;
var ratio = lowerDifference / minnedMax;
return ratio * 100;
}
I believe this is simpler, but still had to pause in order to decipher what lowerDifference
and minnedMax
were. I can't seem to come up with a good name for lowerDifference
, but I believe that score
may be slightly better. minnedMax
is easier. max - min
is equal to the range
between max
and min
.
function zeroToOneHundredScore(number, min, max) {
if (number < min) {
return 0;
}
if (number > max) {
return 100;
}
var score = number - min;
var range = max - min;
var ratio = score / range;
return ratio * 100;
}
Next up, calculateSumAnswerScore
. What is sumAnswerScore
? It turns out that it is the sum of the scores of all the answers for a given question, but this was not immediately obvious from the function name. I would recommend calculateTotalAnswerScore
(or Reputation
/ Rep
instead of Score
). calculateSum
sounds redundant to me. Another alternative name could be sumAnswerScores
(I think this is probably better as it indicates that there are multiple answers)
var sumAnswerScore = 0;
$.each(data.answers, function(index, value) {
sumAnswerScore = sumAnswerScore + value["score"];
});
This is a perfect place to use Array.prototype.reduce
. Here is an ES5 version with the same result:
var score = data.answers.reduce(function(carry, answer) {
return carry + answer.score;
}, 0);
Or, if you prefer, use arrow functions to slightly reduce the amount of code.
var score = data.answers.reduce((carry, answer) => carry + answer.score, 0);
var noZeroSumAnswerScoreXXX = ...
- This needs a better name. XXX
provides no value to anyone reading the code and we already know we are talking about the sum of the answer score from the function name. A better name would just be positiveScore
or clampedScore
as this score also cannot be less than zero. Alternatively, just remove this variable entirely and return the result of the ternary operation. The ternary operation also uses too many parenthesis. At the very least, remove the parenthesis surrounding the entire expression and the two results to make it easier to read.
With these changes made, here's how I would write the method using the ES5 reduce
for maximum compatibility.
function sumAnswerScores(question) {
var score = question.answers.reduce(function(carry, answer) {
return carry + answer.score;
}, 0);
return Math.min(0, score);
}
Next up, calculateCompetitionRating
, good function name! I know exactly what this method will do. The biggest problem this function has is not the line labeled "duplicate code". It is the massive formula assigned to finalScore
. What happens when/if you add the acceptedAnswer
calculation back into the score calculation and forget to change 5
to 6
? This is a good opportunity to create a helper method to take the average of several numbers.
function mean(numbers) {
return numbers.reduce(function(carry, num) {
return carry + num;
}, 0) / numbers.length;
}
With this helper function, the function can be simplified slightly.
function calculateCompetitionRating(data) {
var noZeroSumAnswerScore = calculateSumAnswerScore(data);
var deltaTimeCreation = epochSeconds - data.question.creation_date;
var deltaTimeLastActivity = epochSeconds - data.question.last_activity_date;
var answerCount = data.question.answer_count;
var views = data.question.view_count;
var finalScore = mean([
zeroToOneHundredScore(-deltaTimeCreation, -300000, 0) +
zeroToOneHundredScore(-deltaTimeLastActivity, -100000, 0) +
zeroToOneHundredScore(-answerCount, -2.35, 0) +
zeroToOneHundredScore(-noZeroSumAnswerScore, -4, 0) +
zeroToOneHundredScore(views / deltaTimeCreation, 0, 0.0005)
])
appendHTML(data.question.question_id, "Competition Rating: " + Math.round(finalScore));
}
I still believe this can can be improved. The answerCount
and view_count
variables really don't add anything to the clarity of this function besides providing a shortcut for use later. As the shortcut is only used once, there isn't much point in creating it. On the other hand, data.question
does appear several times and can be simplified with a shortcut to question
.
function calculateCompetitionRating(data) {
var question = data.question;
var totalAnswerScores = sumAnswerScores(data);
var deltaTimeCreation = epochSeconds - question.creation_date;
var deltaTimeLastActivity = epochSeconds - question.last_activity_date;
var score = mean([
zeroToOneHundredScore(-deltaTimeCreation, -300000, 0),
zeroToOneHundredScore(-deltaTimeLastActivity, -100000, 0),
zeroToOneHundredScore(-question.answer_count, -2.35, 0),
zeroToOneHundredScore(-noZeroSumAnswerScore, -4, 0),
zeroToOneHundredScore(question.view_count / deltaTimeCreation, 0, 0.0005)
]);
appendHTML(question.question_id, "Competition Rating: " + Math.round(score));
}
Feedback for calculateRepProfitEstimate
would essentially be identical to calculateCompetitionRating
so I'll leave refactoring this function to you. I will also skip addQandAtoDataSet
as I'm going to recommend removing it.
The transferJSONtoDataSet
, synthesizeAllData
functions are best looked at after reviewing the last bit of code in the IIFE.
var $questionsOnPage = $(".question-summary");
$questionsOnPage.each(function (index) {
var $currentQuestion = $(this);
var questionID = $currentQuestion.attr("id");
questionID = questionID.substr(questionID.lastIndexOf("-") + 1); // The id of a question is at the end of the question's id attribute. This gets the id from the id.
questionsToQuery.push(questionID);
});
Currently, this code relies on the questionsToQuery
variable which is defined over 30 lines earlier with 3 functions in between. This isn't a very good idea as it results in a lot of searching to figure out where questionsToQuery
came from. It would be better to just get an array of question ids and assign the result immediately. Thankfully, jQuery can help with this as it provides a map
function on the object returned by $()
.
var questionsToQuery = $(".question-summary").map(function(index, question) {
// Note: `question` is the element itself, not a jQuery wrapper.
var id = question.id;
id = id.substr(id.lastIndexOf('-') + 1);
return id;
}).get();
Now that we have the IDs, there's no need to wait for the api to return the question data before fetching the answer data. This is a great place to use promises. As the code currently uses $.get
I'll continue using it, but I would recommend looking into the built in Fetch API. I'm not sure why you are requiring an order
/ sort
in the API request as you then disregard this information, but I'll leave that in for now as there may be API specific requirements I am unaware of.
// Perform the requests at the same time, resolve when both requests resolve
Promise.all([
$.get("https://api.stackexchange.com/2.2/questions/" + questionsToQuery.join(";"), {
order: 'desc',
sort: 'activity',
site: 'stackoverflow'
}).promise(),
$.get("https://api.stackexchange.com/2.2/questions/" + questionsToQuery.join(";") + "/answers", {
order: 'desc',
sort: 'activity',
site: 'stackoverflow'
}).promise()
]).then(function(data) {
transferJSONtoDataSet(data[0], data[1]);
synthesizeAllData();
});
Now back to transferJSONtoDataSet
, first up, there's no need to wrap so many loops! If you loop through everything in questionsData.items
you will have looped through all questions. In addition to this, looping through the answers can be simplified by using the Array.prototype.filter
method.
function transferJSONtoDataSet(questions, answers) {
questions.items.forEach(function(question) {
var thisQuestionsAnswers = answers.items.filter(function(answer) {
return answer.question_id == question.question_id;
});
addQandAtoDataSet(question, thisQuestionsAnswers);
});
}
Not that that has been cleaned up a bit, I don't see much value in keeping addQandAtoDataSet
as all it does is push to an array. The same goes for synthesizeAllData
. We can just call calculateRepProfitEstimate
and calculateCompetitionRating
here. Since the function no longer transfers any data, it's time to rename it to something more descriptive, I choose scoreQuestions
, though there is arguably a better name.
function scoreQuestions(questions, answers) {
questions.items.forEach(function(question) {
var thisQuestionsAnswers = answers.items.filter(function(answer) {
return answer.question_id == question.question_id;
});
var data = {
question: question,
answers: thisQuestionsAnswers
};
calculateRepProfitEstimate(data);
calculateCompetitionRating(data);
});
}
All together now. This could still be improved. What I would do next is try to only pass each function the data that it requires. For example sumAnswerScores
only needs an array of answers, not a wrapper containing both the question and the answers.
(function(){
"use strict";
function getEpoch() {
return Math.floor(Date.now() / 1000);
}
function transformQuestionScore(input) {
return Math.min(input, 15) * 0.01485;
}
function transformQuestionAskerRep(input) {
return Math.min(input, 750) * 0.00002664;
}
function transformQuestionViewVelocity(input) {
return Math.min(input, 0.01) * 405.8;
}
function transformQuestionViews(input) {
return Math.pow(0.025 * Math.min(input, 5000), 0.5);
}
function appendHTML(id, content) {
$("#question-summary-" + id).append("<div style='clear: both;'><strong>" + content + "</strong></div>");
}
function zeroToOneHundredScore(number, min, max) {
if (number < min) {
return 0;
}
if (number > max) {
return 100;
}
var score = number - min;
var range = max - min;
var ratio = score / range;
return ratio * 100;
}
function sumAnswerScores(question) {
var score = question.answers.reduce(function (carry, answer) {
return carry + answer.score;
}, 0);
return Math.max(0, score);
}
function mean(numbers) {
return numbers.reduce(function (carry, num) {
return carry + num;
}, 0) / numbers.length;
}
function calculateCompetitionRating(data) {
var question = data.question;
var totalAnswerScores = sumAnswerScores(data);
var deltaTimeCreation = getEpoch() - question.creation_date;
var deltaTimeLastActivity = getEpoch() - question.last_activity_date;
var score = mean([
zeroToOneHundredScore(-deltaTimeCreation, -300000, 0),
zeroToOneHundredScore(-deltaTimeLastActivity, -100000, 0),
zeroToOneHundredScore(-question.answer_count, -2.35, 0),
zeroToOneHundredScore(-totalAnswerScores, -4, 0),
zeroToOneHundredScore(question.view_count / deltaTimeCreation, 0, 0.0005)
]);
appendHTML(question.question_id, "Competition Rating: " + Math.round(score));
}
function calculateRepProfitEstimate(data) {
var question = data.question;
var views = question.view_count;
var deltaTimeCreation = getEpoch() - question.creation_date;
var averageAnswerScore = sumAnswerScores(data) / question.answer_count;
var rawRepProfit = mean([
transformQuestionScore(question.score),
transformQuestionAskerRep(question.owner.reputation),
transformQuestionViewVelocity(views / deltaTimeCreation),
transformQuestionViews(views),
Math.min(averageAnswerScore, 20)
]) * 15;
var owner = question.owner;
var acceptRate = owner.hasOwnProperty("accept_rate") ? owner.accept_rate : 50;
rawRepProfit += (acceptRate / 100) * 15;
appendHTML(question.question_id, "Estimated Reputation Profit: " + Math.round(rawRepProfit) + ((rawRepProfit >= 90) ? " or more" : ""));
}
function scoreQuestions(questions, answers) {
questions.items.forEach(function (question) {
var thisQuestionsAnswers = answers.items.filter(function (answer) {
return answer.question_id == question.question_id;
});
var data = {
question: question,
answers: thisQuestionsAnswers
};
calculateRepProfitEstimate(data);
calculateCompetitionRating(data);
});
}
var questionsToQuery = Array.from(document.querySelectorAll(".question-summary")).map(function(question) {
var id = question.id;
id = id.substr(id.lastIndexOf("-") + 1);
return id;
});
// Perform the requests at the same time, resolve when both requests resolve
Promise.all([
$.get("https://api.stackexchange.com/2.2/questions/" + questionsToQuery.join(";"), {
order: 'desc',
sort: 'activity',
site: 'stackoverflow'
}).promise(),
$.get("https://api.stackexchange.com/2.2/questions/" + questionsToQuery.join(";") + "/answers", {
order: 'desc',
sort: 'activity',
site: 'stackoverflow'
}).promise()
]).then(function(data) {
scoreQuestions(data[0], data[1]);
});
})();
Math.max()
function in JavaScript? I don't see the need oflimit
in light of that. \$\endgroup\$Math.max
doesn't work. I thinkMath.min
is the correct one. \$\endgroup\$