Simple Book List App I've built a simple Book list app. It allows users to enter the name, author and isbn of a book into a form. Then when this is submitted successfully, the details get added to a new row in the table.

I've tried to make the code as optimal as possible. Any feedback would be greatly appreciated.

const bookList = document.getElementById('book-list');


class Book {
  constructor(title, author, isbn){
    this.title = title;
    this.author = author;
    this.isbn = isbn;
  }

  addBook(){
    const bookList = document.getElementById('book-list');
    const bookItem = document.createElement('tr');

    bookItem.innerHTML =`
      &lt;td&gt;${this.title}&lt;/td&gt;
      &lt;td&gt;${this.author}&lt;/td&gt;
      &lt;td&gt;${this.isbn}&lt;/td&gt;
      &lt;td&gt;&lt;button type="submit" class="delete"&gt;X&lt;/button&gt;&lt;/td&gt;`;

    return bookList.appendChild(bookItem);
  }

  static deleteBook(target){
    target.originalTarget.parentElement.parentElement.remove();
  }
}

// Event Listener: Form Submit
form.addEventListener('submit', function(e){
  const bookFields = getFields();

  if(!validateForm(bookFields)) {
    showNotification('Error', 'error');
    e.preventDefault();
    return false;
  }

  const book = new Book(
    bookFields.title, 
    bookFields.author, 
    bookFields.isbn
  );

  book.addBook(book);

  showNotification('Success', 'success');
  this.reset(); // Reset Form

  e.preventDefault();
});

// Event Listener: Delete Book
bookList.addEventListener('click', function(e){
  Book.deleteBook(e);
  showNotification('Deleted Item', 'success');
});


function getFields(){
  return fields = {
    'title': document.getElementById('title').value,
    'author': document.getElementById('author').value,
    'isbn': document.getElementById('isbn').value
  };
}

function validateForm(fields){
  for(let [key, value] of Object.entries(fields)){
    if(value === '') return false;
  }
  return true;
}

// Notification
function showNotification(message, type){
  const container = document.querySelector('.container');
  const alert = document.createElement('div');

  alert.classList = `${type}`;
  alert.appendChild(document.createTextNode(message));

  container.insertBefore(alert, form);

  setTimeout(function(){
    alert.remove();
  }, 1500);
}```
Some possible improvements:

You select the bookList multiple times. You only need to select it once, in the outer scope, and let lexical scoping do the rest - you can remove the line that re-declares it without issues.

Inserting characters from user input directly into a string of HTML markup can occasionally result in unexpected elements appearing when the input contains &lt; brackets. (This can also allow for arbitrary code execution)



const bookItem = document.querySelector('tr');
const title = 'Title &lt;Subtitle in brackets&gt;';
const author = '&lt;img src onerror="alert(\'evil\');"';
bookItem.innerHTML =`
  &lt;td&gt;${title}&lt;/td&gt;
  &lt;td&gt;${author}&lt;/td&gt;
`;
&lt;table&gt;
  &lt;tr&gt;
  &lt;/tr&gt;
&lt;/table&gt;




Either escape &lt; brackets first, or insert the &lt;td&gt;s without content, then select them afterwards and assign to their textContent (which won't have any escaping issues).

&lt;td&gt;${this.title.replace(/&lt;/g, '&amp;lt;')}&lt;/td&gt;
&lt;td&gt;${this.author.replace(/&lt;/g, '&amp;lt;')}&lt;/td&gt;
&lt;td&gt;${this.isbn.replace(/&lt;/g, '&amp;lt;')}&lt;/td&gt;
&lt;td&gt;&lt;button type="submit" class="delete"&gt;X&lt;/button&gt;&lt;/td&gt;`;


The return value from addBook is not used, so there's no need for it to return a value. Unless you're intending consumers of Book to use the created &lt;tr&gt;, you can remove the return from the end of the function.

Your deleteBook function is odd - whenever anything in the bookList is clicked, it will remove the grandparent element. This does not seem desirable. You probably want to remove a &lt;tr&gt; only when the X button is clicked. Add an event listener to the button instead:

addBook(){
  const tr = document.createElement('tr');
  tr.innerHTML =`
  &lt;td&gt;${this.title.replace(/&lt;/g, '&amp;lt;')}&lt;/td&gt;
  &lt;td&gt;${this.author.replace(/&lt;/g, '&amp;lt;')}&lt;/td&gt;
  &lt;td&gt;${this.isbn.replace(/&lt;/g, '&amp;lt;')}&lt;/td&gt;
  &lt;td&gt;&lt;button type="submit" class="delete"&gt;X&lt;/button&gt;&lt;/td&gt;`;
  const button = tr.querySelector('button');
  button.addEventListener('click', () =&gt; {
    tr.remove();
    showNotification('Deleted Item', 'success');
  });
  bookList.appendChild(tr);
}


You could also keep using your listener on the whole bookList, but you'd have to make sure that the clicked element is the X button before removing elements - check if the target .matches('.delete'). (but even then, it's strange for that functionality to be outside of the Book class - it would make more sense for it to be encapsulated)

If you do use the delegation method, use event.target, not event.originalTarget (which is non-standard and only works on Firefox - on any other browser, it will throw an error). (also, best to name the variables appropriately - you're passing the event to deleteBook, not the event.target, so calling the parameter target is confusing)

You can make the submit listener a bit less repetitive by calling e.preventDefault on the first line of the function, that way you don't have to write it twice later, nor do you have to worry about accidentally not calling it in case you add more logical paths to the function. (And if you call preventDefault(), there's no need to return false, nothing needs to be returned)

In getFields, you're implicitly creating a global variable fields. This doesn't do anything useful, and will throw an error in strict mode (which you should be using). Just remove the fields = part:

function getFields(){
  return {
    'title': document.getElementById('title').value,
    'author': document.getElementById('author').value,
    'isbn': document.getElementById('isbn').value
  };
}


Doesn't matter much here, but if you had more fields, to be less repetitive, you could replace that with:

function getFields() {
  return Object.fromEntries(
    ['title', 'author', 'isbn'].map(
      id =&gt; [id, document.getElementById(id).value]
    )
  );
}


It's good to prefer const whenever possible - using let is a warning to future readers of the script (including yourself) that you may be reassigning the variable later. So, you could replace

for(let [key, value] of Object.entries(fields)){


with

for(const [key, value] of Object.entries(fields)){


Or, even better, since you don't care about the keys at all, only that none of the values are empty:

function validateForm(fields){
  return Object.values(fields).every(Boolean);
}


When alert is used as an identifier in Javascript, it usually refers to window.alert. Calling a different variable alert can be confusing. Consider changing

const alert = document.createElement('div');


to something like alertDiv.

Template literals look nicer than concatenation with +, but if there's no concatenation going on, and there aren't any ' or " characters causing escaping issues, there's no point in a template literal. Replace

alertDiv.classList = `${type}`;


with

alertDiv.classList = type;


But the classList is generally understood to be a read-only DOMTokenList. While you can change the class of an element by assigning to the classList property, it looks quite odd. If you want to assign a class, either use classList.add, or (if you don't care about overwriting the existing class) assign to the className property:

alertDiv.className = type;


Using document.createTextNode is often unnecessarily verbose. You can just assign to the textContent of the element instead. Replace

alertDiv.appendChild(document.createTextNode(message));


with

alertDiv.textContent = message;