Manipulating the browser history

Question

I created a table with the ability to edit with popup.

This part of the code, which includes:

1. the ability to send URL to other users with the subsequent opening of the same state (the popup window is empty or with data to be edited) I get the current URL and then I get data from the server. And if the URL includes ID of the data to be editing, I open popup with this data or the empty popup.
2. the ability to work the browser using the buttons "back" and "forward". I used the history.pushState method.

I use the Fetch API for requests to the server. To build the table and the popup, I used jQuery. But in the part of the code with browser history I can't find a way to use jQuery, only pure JS.

I will appreciate any advice about code.

const getLink = window.location.hash.slice(1) // to get the current URL witout hash

fetch('http://localhost:3000/names') // to get the data from the server
    .then((response) => response.json()) 
    .then((_names) => { 
      names = _names //the data
      drawTable() // update table according to data
      if (getLink != 0 && getLink != 'add-new'){ //if URL includes ID of the data
        idEdit = getLink //assign ID from URL as an ID to edit
        addEditValue() // function for popup opening with the data to be editing
      }
    })

 if (getLink == 'add-new') {openAddForm()} //open the empty popup

 top.addEventListener('click', function(e){ // listen a click on the page
    if(e.target == $('#add-new') || e.target.className == 'edit-btn'){
    //if a click on the button for opening empty popup or popup with the data to be editing
    e.preventDefault() 
    const data = e.target.id
    const url = "/#" + data
    e.target.setAttribute('href', url)
    history.pushState(data, null, url)
  } else if(e.target.className != 'input-form-body') {
    //if you click after closing the pop-up window, then clear the URL
    window.location.hash = ""
  }
  e.stopPropagation()
  }, false)


window.addEventListener('popstate', function(e){
  const character = e.state
  if(character == null){
    closeForm() // close popup
    } else {
      idEdit = character
      addEditValue() //open popup with the data to be editing
    }
  })

Answer 1

An appropriate term for this code is spaghetti code – it's tightly intertwined with other parts of the app, so one cannot easily take out a part of it and reason about it without thinking about the rest of the app. Just like it's hard to pull out one single spaghetti from a bowl.

It's not inherently bad, as long as the program is really small (< 10 lines). But as your program grows, you're bound to experience that it's getting harder and harder to reason about your program and make changes in it without breaking things. You might even reach a point where you're afraid to make any changes, as things are so likely to break.

I guess one needs to go through this to really start to look for better ways of organizing the code. So don't feel bad about yourself. Everybody starts out like this. If you want to improve, look out for books like "Pragmatic programmer" or "Clean code". Or search the web for "JavaScript style guide" and "Software engineering principles".

To answer in more detail...

I find it really hard to understand what this code does:

• Seems that it's been extracted out of a larger file, that could give us some context.
• It calls into various functions like drawTable(), addEditValue() which aren't defined in here.
• It assigns to some global variables like names and idEdit, which are also not defined in here.

Several style issues make the code hard to read:

• It's sloppily indented – sometimes 4, sometimes 2 spaces, sometimes no indentation at all.
• Some lines end with semicolon, others don't.

• The comments are sometimes misleading, like:

  // to get the current URL witout hash
  const getLink = window.location.hash.slice(1)
  

  It actually does the opposite – it returns the hash-part of the URL.

• At other times the comments state what's already obvious from the code:

  // listen a click on the page
  top.addEventListener('click', function(e){
  

• The name getLink looks like a function name, but it's not a function. A good style is to only name functions with verbs (as they do things).

• The name data is meaningless. All variables contain data. Naming a variable data is just as helpful as naming it variable.
• The name character is very confusing. Previously you pushed e.target.id to the history state (naming it data). And then in popstate you get back the history state and call it character. If it's a character, then call it so consistently in both places. But it looks like it's not just a single character, because you assign it to idEdit – so it seems to be some sort of ID instead.

Regarding your question:

But in the part of the code with browser history I can't find a way to use jQuery, only pure JS.

That's absolutely fine. jQuery is a library for doing DOM manipulations (for interacting with the HTML on the page). You shouldn't use it for other things besides that.

Answer 2

A couple other things I noticed that weren't mentioned in Rene's answer:

• The arrow functions don't need to have parentheses around single parameter - so lines like these:

  .then((response) => response.json()) 
  .then((_names) => { 
  

  could be written like this:

  .then(response => response.json()) 
  .then(_names => { 
  

• The jQuery function returns a jQuery object - i.e. a collection of DOM elements. Thus the first condition in this line:

   if(e.target == $('#add-new') || e.target.className == 'edit-btn'){   
  

  will never evaluate to true because e.target would just be a single DOM element.

document.addEventListener('DOMContentLoaded', _ => {
  var top = document.getElementById('top');
  var message = document.getElementById('message');

  top.addEventListener('click', e => {
    var targetIsAddNew = e.target == $('#add-new');
    message.innerHTML = 'target is add new: ' + targetIsAddNew
  });
});

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<fieldset id="top">
  <button id="add-new">
  add new
  </button>
</fieldset>
<div id="message">

</div>

• the click handler on top has e.preventDefault() in the first conditional block, and then e.stopPropogation()- that doesn't seem wrong but perhaps only the latter is needed.