3
\$\begingroup\$

I am working on a choice-based simulation and players have to choose between different locations, I am pushing those choices to an array locationLog and my MakeStats() function below builds an array from it. I tend to break code into bits because it helps me, but want to make sure others can understand it.

var hasAttorney = false;
var hasMoney = true;
var hasAC = true;
var currentlocation = "home";
var locationLog = []; //push locations

//Builds a JSON object with game stats
function MakeStats() {
    let home = 0,
        motel = 0,
        relative = 0,
        shelter = 0,
        apartment = 0;

    for (var i = 0; i < locationLog.length; i++) {
        if (locationLog[i] === "home")
            home++;
        else if (locationLog[i] === "motel")
            motel++;
        else if (locationLog[i] === "relative")
            relative++;
        else if (locationLog[i] === "shelter")
            shelter++;
        else if (locationLog[i] === "apartment")
            apartment++;
    }

    let tempJSON = {
        "location": {
            "home": home,
            "motel": motel,
            "relative": relative,
            "shelter": shelter,
            "apartment": apartment
        },
        "moneyLeft": money,
        "ACLeft": AC,
        "finalDestination": currentlocation,
        "hasAttorney": hasAttorney,
        "hasAC": hasAC,
        "hasMoney": hasMoney
    };
    statJSON = tempJSON;
    localStorage.setItem("playerStats", JSON.stringify(statJSON));
    console.log(statJSON);
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code review! Some variables are declared in that function but others are not - e.g. AC, money, locationLog, etc. Where are those defined? Can you update the code sample to provide sample values for those? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 19 '18 at 15:39
  • \$\begingroup\$ updated the variables, the JSON works just fine. \$\endgroup\$ – comphonia Dec 19 '18 at 16:35
  • \$\begingroup\$ okay I am guessing money is declared something like var money = 0 and AC something like var AC = 0;? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 19 '18 at 16:39
2
\$\begingroup\$

Feedback

Simplifying block of if statements

The set of if statements is a bit repetitive, especially since the values match the property names of the nested JSON object that is stored in localStorage. A simpler way to implement this would be to declare tempJSON first, with the nested location values at 0, then iterate through locationLog and increment the appropriate nested value. One can use Object.hasOwnProperty():

if (tempJSON.location.hasOwnProperty(locationLog[i])) {
    tempJSON.location[locationLog[i]]++;

Or use the in operator:

if (locationLog[i] in tempJSON.location) {
    tempJSON.location[locationLog[i]]++;

Both will work the same. For the discussion of the difference, see answers to if (key in object) or if(object.hasOwnProperty(key) on SO.

Iterating over locationLog

Since your code is using features like let, it can also utilize the for...of loop construct instead of setting up variable i and using it to index into the array.

for (const location of locationLog) {
    if (location in tempJSON.location) {

Default to using const for variables to avoid unintentional re-assignment

Variables that should not be re-assigned, like tempJSON can be declared with const instead of let. If you determine later that re-assignment should be allowed (e.g. in a loop or conditional expression) then use let.

variable statJSON

That may be a global variable or else you just didn't include the local declaration (and assignment) but if it is a global variable then consider making it a local variable. In the scope of the code you included, it appears superfluous...

Rewritten code

See the snippet below utilizing the feedback above.

var hasAttorney = false;
var hasMoney = true;
var money = 0;
var AC = 1;
var hasAC = true;
var currentlocation = "home";
var locationLog = ["relative", "shelter", "relative"]; //push locations

//Builds a JSON object with game stats
function MakeStats() {
    const tempJSON = {
        "location": {
            "home": 0,
            "motel": 0,
            "relative": 0,
            "shelter": 0,
            "apartment": 0
        },
        "moneyLeft": money,
        "ACLeft": AC,
        "finalDestination": currentlocation,
        "hasAttorney": hasAttorney,
        "hasAC": hasAC,
        "hasMoney": hasMoney
    };
    for (const location of locationLog) {
      if (location in tempJSON.location) {
        tempJSON.location[location]++;
      }
    }
    
    //localStorage not allowed in SE snippets for security reasons
    //localStorage.setItem("playerStats", JSON.stringify(tempJSON));
    console.log(tempJSON);
}
MakeStats();

\$\endgroup\$
  • 1
    \$\begingroup\$ much better, just getting acquainted with ES6. It's a bit weird an object with const lets you update values. \$\endgroup\$ – comphonia Dec 19 '18 at 19:22
  • 1
    \$\begingroup\$ Yes - "The const declaration creates a read-only reference to a value. It does not mean the value it holds is immutable, just that the variable identifier cannot be reassigned." - MDN documentation for const \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 19 '18 at 19:24
  • \$\begingroup\$ for (let location of locationLog) could be for (const location of locationLog) if location is not going to be reassigned \$\endgroup\$ – Blindman67 Dec 20 '18 at 3:20
  • \$\begingroup\$ Good on ya! the documentation on MDN even says so ... but then the subsequent examples about iterating over a string use let though the value isn't reassigned inside the loop block- SERENITY NOW!! Maybe I should edit those examples \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 20 '18 at 5:15
3
\$\begingroup\$

Some extra points that Sam Onᴇᴌᴀ's good review skipped.

Capitalizing function names

Don't capitalize functions if they are not used to create an object. In JavaScript we only capitalize functions or objects that can be instantiated with the new token. Thus MakeStats should be makeStats.

However if you were to return the object statJSON then you could use the name MakeStats to indicate that it instantiates an object. You can then create the object in 3 ways, each of which return the same object.

    // last 2 lines of MakeStats to be
    return statJSON;
}

const s1 = new MakeStats();
const s2 = new MakeStats;  // with new objects () is not needed if there are no arguments
const s3 = MakeStats();

Quoted property names

I see this a lot, when people create an object that is to be stringified. For some reason quoted property names are used. Doing so is just adding unneeded noise to the code.

The only time you need to use quotes to define a property is if the property is not a valid javascript variable name. e.g. {data-one: "one"} will throw an error as data-one is not a valid name, you would thus write it {"data-one": "one"}

Shorthand property names (ES2015)

Use object property shorthand if you are adding existing named variables to an object.

You have

let tempJSON = {
    "location": {
        "home": home,
        "motel": motel,
        "relative": relative,
        "shelter": shelter,
        "apartment": apartment
    },
    "moneyLeft": money,
    "ACLeft": AC,
    "finalDestination": currentlocation,
    "hasAttorney": hasAttorney,
    "hasAC": hasAC,
    "hasMoney": hasMoney
};

can be written as

const tempJSON = {
    location: {home, motel, relative, shelter, apartment},
    moneyLeft: money,
    ACLeft: AC,
    finalDestination: currentlocation,
    hasAttorney, 
    hasAC,
    hasMoney
};

the resulting object is identical to the longhand version.

localStorage

You can use simpler notation when accessing localStorage.

You have

localStorage.setItem("playerStats", JSON.stringify(statJSON));

is identical to

localStorage.playerStats = JSON.stringify(statJSON);

The same applies to reading from localStorage

stats = localStorage.playerStats; // Note If that data is not there you get undefined
                                  // Note All data from local storage is a string

Don't trust localStorage

There is no way to guarantee that localStorage content written in one session will be available next time, nor can you trust the data you read back, and many 3rd party extensions have full access to localStorage.

If the data is important, requires trust, or contains personal client data it should be saved on the server.

Warning about cyclic data

JSON.stringify will throw a TypeError: "Converting circular structure to JSON" if the data contains cyclic references.

Cyclic references are everywhere in JavaScript so if you are stringifying data from unknown or unsure sources is pays to wrap the stringify in a try catch so at least you can recover gracefully. As your function is using data from outside its control it would pay to be safe (unless you are in full control of all data).

\$\endgroup\$
  • \$\begingroup\$ Don't capitalize functions if they are not used to create an object. In JavaScript we only capitalize functions or objects that can be instantiated with the new token. I considered Modifying the second sentence to begin "In idiomatic JavaScript..." but am not sure what you would want.... it may be wise to mention conventions/style guides there... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 20 '18 at 5:34

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.