Game where you guess whether an article is from theOnion or notTheOnion

How can I make my code better?

```
function generateRandNum(min, max) {
minimum = Math.ceil(min);
maximum = Math.floor(max);
return Math.floor(Math.random() * (maximum - minimum)) + minimum;
}

function generateNewPost(json_data) {
var rand_num = generateRandNum(0, json_data.length);
console.log("generateNew length: ", json_data.length);
var rand_post = json_data[rand_num];
return rand_post;
}

var post_title = document.getElementById("post-title");
var onion_button = document.getElementById("onion-chosen");
var not_onion_button = document.getElementById("not-onion-chosen");
var score_display = document.getElementById("score");

var req = new XMLHttpRequest();
var json_url = "https://api.myjson.com/bins/s474n";
var score = 0;

req.open('GET', json_url);
req.onload = function() {

if (req.status === 200) {
var json_data = JSON.parse(req.responseText);
var rand_post = generateNewPost(json_data);

post_title.innerHTML = rand_post.content;
score_display.innerHTML = score;

onion_button.onclick = function() {
if (rand_post.sub === "theonion") {
score++;
score_display.innerHTML = score;
} else {
score--;
score_display.innerHTML = score;
}

// update title
rand_post = generateNewPost(json_data);
var index = json_data.indexOf(rand_post);

console.log("Index: ", index);
console.log("Length of json_data: ", json_data.length);
json_data.splice(index, 1);
console.log("Length of json_data after splice: ", json_data.length);

post_title.innerHTML = rand_post.content;

}

not_onion_button.onclick = function() {
if (rand_post.sub === "nottheonion") {
score++;
score_display.innerHTML = score;
} else {
score--;
score

Avoid repetitive code

Dair has covered this already above. Basically, whenever code looks like it's been copy-pasted, it needs to be rewritten.

Another case of unnecessary repetition is the lines:

post_title.innerHTML = rand_post.content;
    score_display.innerHTML = score;

You use these to display the first post and the starting score. These two lines are also present in your onclick methods. This means if you or someone else needed to change how these elements are displayed, for example, they would have to repeat making the same changes to your code but in multiple places. Well-written code never forces anyone to do this. Essentially, each task accomplished by your code should be written down once, and only once!

Use your data when you have it

Your generateNewPost function accesses json_data using a random number as the index:

var rand_post = json_data[rand_num];

But after calling generateNewPost, you search json_data again for the index that 'generateNewPost' has already used:

var index = json_data.indexOf(rand_post);

As a result, your code does an unnecessary search of json_data every time the user clicks an answer. To save doing this search, generateNewPost should return the index as part (or all) of its output:

function generateNewPost(json_data) {
    var rand_num = generateRandNum(0, json_data.length);
    var rand_post = json_data[rand_num];
    return [rand_post, rand_num];
}

Which would be accessed like this:

var new_post = generateNewPost(json_data)
var rand_post = new_post[0]
var index = new_post[1]

This is not necessary here as shown below. But it might be helpful to remember it when creating more complex functions.

Keep things abstract until you need them

The two actions you want for json_data, that is, reading a random entry, and deleting an entry, both need only an index. So it makes sense to keep an integer index as your rand_post, rather than passing around the post content as a string. Further, as generating random numbers and accessing entries in a JSON object are fairly standard methods, they do not need to be wrapped in a function.

tl;dr

Your code can be simplified by:

• Using a single function to initialize the display and handle both onclick events.

• Trusting future readers of you code to understand a one-line version of your generateNewPost function, which is fairly standard.

As below:

var post_title = document.getElementById("post-title");
var onion_button = document.getElementById("onion-chosen");
var not_onion_button = document.getElementById("not-onion-chosen");
var score_display = document.getElementById("score");

var req = new XMLHttpRequest();
var json_url = "https://api.myjson.com/bins/s474n";
var score = 0;
var index = -1;
var options = ["nottheonion", "theonion"]

req.open('GET', json_url);
body.onload = function() {
    var json_data = JSON.parse(req.responseText);
    onion_button.onclick = update(1);
    not_onion_button.onclick = update(0);
    update(null);
}

update = function(user_entry) {
    if (user_entry != null) {
        if (options[user_entry] == json_data[index].sub) {
            score++;
        } else {
            score--;
        }
        json_data.splice(index, 1);
    }
    index = parseInt((Math.random() * (json_data.length - 1), 10)
    post_title.innerHTML = json_data[index].content;
    score_display.innerHTML = score;
}

Note:
It can also help anyone who needs to modify or work with your code to include a comment block showing the expected format of any objects passed from elsewhere, eg. from your url api.myjson.com. It might look like this:

/*
[
  { content: #string#, sub: "notheonion" / "theonion" }, ...
]
*/

StackExchange Code Review Q#150904, answer score: 4