3
\$\begingroup\$

I am completely new to JavaScript and I will be grateful for your comments on my Electron app (electron is a JS framework allowing to create of desktop apps).

I needed to create an application that stores a small database and allows me to do simple operations on the data. I just include a small fraction of that app here that embraces just basic CRUD functionality on one table. The table is serialized into a teams.json file with the following structure:

[
    {
        "name": "Blue",
        "gold": 0,
        "food": 0,
        "fame": 0
    },
    {
        "name": "Red",
        "gold": 0,
        "food": 0,
        "fame": 0
    },
    {
        "name": "Green",
        "gold": 0,
        "food": 0,
        "fame": 0
    }
]

App has just two screens. The first screen is just a whole table. The second screen allows modifications (see the picture).

enter image description here

The edit screen has two main parts:

  1. Read from JSON and construction of the HTML
  2. Serialization into JSON

1. Read and construction

function create_team(team) {
  var div_team = document.createElement("div");
  div_team.setAttribute("id", "team");
  var div_html_string = "<textarea class=\'divedit_str\'>" + team.name + "</textarea>"
  div_html_string += "<textarea class=\'divedit\'>" + team.gold + "</textarea>";
  div_html_string += "<textarea class=\'divedit\'>" + team.food + "</textarea>";
  div_html_string += "<textarea class=\'divedit\'>" + team.fame + "</textarea>";
  div_team.innerHTML = div_html_string;

  var remove_button = document.createElement("button");
  remove_button.setAttribute("class", "remove")
  remove_button.innerHTML = "Remove";
  remove_button.addEventListener("click", (e) => {
    e.target.parentElement.remove();
  })
  div_team.appendChild(remove_button)

  return div_team;
}

function loading_panels() {
  teams = JSON.parse(fs.readFileSync('./app/teams.json').toString());

  //// loading view teams panel
  teams.forEach((team) => {
    div_teams.appendChild(create_team(team));
  });
}

2. Serialization

button_teams.addEventListener('click', () => {
  var teams_div = document.querySelectorAll("div#team");
  var json_teams = [];
  teams_div.forEach((team) => {
    json_teams.push({ "name": team.children[0].value, "gold": Number(team.children[1].value), "food": Number(team.children[2].value), "fame": Number(team.children[3].value) });
  });
  fs.writeFileSync('./app/teams.json', JSON.stringify(json_teams), function (err) {
    if (err) throw err;
  });
  window.location.replace("./index.html");
});

The working code can be found in this GitHub repository.

Since this is a common MVC problem I'm curious whether there is a solution where I can separate the view and main logic. Please ignore the CSS of the app. I'm not a frontend developer, therefore, I believe the styles are completely wrong.

Thank you for any comments!

\$\endgroup\$
1
+50
\$\begingroup\$

The easy stuff:

  • Remove unnecessary comments. loading view teams panel - This comment adds nothing. If anything, it tells me that your code is not organized properly since you have to add these reminders. In the repo you have many more examples (for example // JSON variable). In my opinion you can remove basically all your comments and have it more readable as a result. Focus on organizing your code instead. Focus on good variable names and good abstractions.
  • You're in electron, so you can use new javascript syntax. Use const/let instead of var. Use arrow functions. Etc.
  • When reading a file synchronously you don't need to use a callback. Also, you should use async/await instead of callbacks when async.
  • You're outputting variable content directly into html. Even for a single user this will cause problems. Escape your output (or use a framework that does it for you, like react).
  • Nit: In javascript it is idiomatic to use camel case. Not a big deal, just pointing it out.

The hard stuff:

When you update your file, you don't reload it or trigger a new render. If you are to scale this up, you need to have a way to do this (or have a proxy cache for the file if you don't want to update it constantly). Let's say you have a second component that uses the same team state (in fact it seems you do in the repo). It won't get updated. It's a good idea to separate the storage/model from the rendering/event listeners/etc. If this were react, you would have events flowing up, triggering model changes, and a new model flowing down, triggering renders. This can be done in a number of ways (see mvc, flux, redux, or react state). In your app you could manually call loading_panels after each update, but you can see how that is not a lasting solution. You could probably get by with a simple storage object: storage.update and either subscribe to the storage for changes, or simply have the storage trigger a full render after each change. The key point here is to separate storage/model from rendering/view.

Some ideas to get you started:

  • Remove the file reader from the renderer, and name the remaining function something to do with render (as opposed to the generic loader that does everything)
  • Make a separate storage solution (that currently is duplicated across multiple components).
  • Have every renderer take its data as an argument
  • Call the necessary renderers whenever the storage is updated.
  • Load the storage just once, at startup
\$\endgroup\$
4
  • \$\begingroup\$ "When you update your file, you don't reload it or trigger a new render", I thought that I'm doing this calling: "window.location.replace("./index.html");" \$\endgroup\$ Jul 26 at 20:47
  • \$\begingroup\$ I did not mention it explicitly, but I want my model to be persisted somewhere after each update (that is important for me). I would like to separate model from view. So you say react is a way to go, right? \$\endgroup\$ Jul 26 at 20:52
  • \$\begingroup\$ You're sort of triggering rendering by reloading the page, but this approach only goes so far. As your page grows, you'll most likely get into situations where one part of the page is out of sync with another. Using react is a solution for rendering and local state (unless you count react context). But in your case you have global state. For this people usually go for something outside of react, like redux (or you can use react context). The idea is the same without react as well though. I would try separating the global storage from rendering as a start. \$\endgroup\$ Jul 27 at 18:31
  • \$\begingroup\$ Thanks. Is it possible to add an example that would illustrate the separation of rendering and model? \$\endgroup\$ Jul 28 at 17:57
1
\$\begingroup\$

MVC

Separating the code into MVC components is possible. Consider the example in this post. As we pointed out in reviews the controller had aspects that would likely have been more appropriate left up the the view. Be careful to separate the three components so each only handles its own responsibilities.

A good example of this separation is demonstrated in the example at the end of the answer by sineemore where the controller calls methods and accesses public properties of the model and view to coordinate when each should do something. The controller doesn’t need to know that the view uses a DOM - the view could potentially be replaced with a view that uses a console if need be and the controller should still perform its responsibilities the same way.

Suggestions

function names

The name create_team almost makes it sound like it makes a team. A more appropriate name might be something like create_row_for_team or as Magnus’s answer recommends: idiomatic JavaScript uses camelCase - e.g. createRowForTeam.

The name loading_panels appropriately describes what it does - almost. Typically function names would have a verb in the imperative form instead of a gerund - e.g. loadPanels.

id attributes

Even though this is an electron app and not a typical HTML page, there is a fundamental aspect to the id attribute: it "must be unique in the whole document."1. Consider using a class attribute instead of id, or else use a unique identifier - possibly existing property or new property incremented from a given integer.

Textarea creation

The code to create the HTML of textareas is a bit repetitive:

var div_html_string = "<textarea class=\'divedit_str\'>" + team.name + "</textarea>"
div_html_string += "<textarea class=\'divedit\'>" + team.gold + "</textarea>";
div_html_string += "<textarea class=\'divedit\'>" + team.food + "</textarea>";
div_html_string += "<textarea class=\'divedit\'>" + team.fame + "</textarea>";

In the past there were discussions about the most efficient way to concatenate strings. Browsers have come a long way in the past decade and that may not really be a major concern anymore - perhaps with Electron it isn't anyway. Also, Ecmascript 6 template literals can help. A script tag could also be used as a template. Or to be consistent with other code in the function document.createElement() could be used - perhaps in a loop for the last three properties:

['gold', 'food', 'fame'].forEach(property => {
    const textarea = document.createElement('textarea');
    textarea.classList.add('divedit');
    textarea.innerText = team[property];
    div_team.appendChild(textarea);
});

If all of those properties should be integers then consider using <input type="number"> so the user can increment the numbers if desired.

['gold', 'food', 'fame'].forEach(property => {
    const input = document.createElement('input');
    input.type = 'number';
    input.value = team[property];
    div_team.appendChild(input);
});

const instead of var

Because Ecmascript-6 features like arrow functions are used, const and let can be used to declare variables. This keeps the scope limited to the block. And using const can be used for any variable that is only assigned once. This can help avoid accidental re-assignment and other bugs.

pushing in forEach()

In the serialization code there are these lines:

var json_teams = [];
teams_div.forEach((team) => {
  json_teams.push({ "name": team.children[0].value, "gold": Number(team.children[1].value), "food": Number(team.children[2].value), "fame": Number(team.children[3].value) });
});

This can be simplified by using the .map() method- then the callback function can simply return the object instead of pushing it into the array manually:

const json_teams = teams_div.map((team) => {
    "name": team.children[0].value, "gold": Number(team.children[1].value), "food": Number(team.children[2].value), "fame": Number(team.children[3].value)
});

or for readability it might be better to add new lines after each comma:

const json_teams = teams_div.map((team) => {
   "name": team.children[0].value,
   "gold": Number(team.children[1].value),
   "food": Number(team.children[2].value),
   "fame": Number(team.children[3].value)
});
\$\endgroup\$

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.