Javascript to show folder and files by calling Web Api

Question

I have this Javascript which call the web api to see the available folder and then the available files. Any comment or code review on my code?

    var divFolder = document.getElementById("rootFolder");
    var divFiles = document.getElementById("files");

    function createFolderElement(data) {
        var li = document.createElement("div");
        li.setAttribute('data-id', data.filePathId);
        li.setAttribute('class', "folder");

        divFolder.appendChild(li);
        li.innerHTML = data.folderName
    }

    function createFileElement(data) {
        debugger
        var li = document.createElement("div");
        li.setAttribute('data-id', data.id);
        li.setAttribute('class', "file");

        divFiles.appendChild(li);
        li.innerHTML = data.fileName
    }

    async function fetchFolder(parentId) {
        document.getElementById('spinner').style.display = 'block'
        divFolder.innerHTML = ''

        const response = await fetch(`https://localhost:44371/api/file/folder/${parentId}`);

        if (!response.ok) {
            const message = `An error has occured: ${response.status}`;
            $("div.message").text(message);
            $("div.message").addClass("message alert alert-warning");
            //throw new Error(message);
        }

        const files = await response.json();
        if (files.length > 0) {
            files.forEach((data) => {
                createFolderElement(data)
            })

            createFolderEventListener();

            document.getElementById('spinner').style.display = 'none'
        }
    }

    async function fetchFiles(filePathId) {
        document.getElementById('spinner').style.display = 'block'
        divFolder.innerHTML = ''

        const response = await fetch(`https://localhost:44371/api/file/files/${filePathId}`);

        if (!response.ok) {
            const message = `An error has occured: ${response.status}`;
            $("div.message").text(message);
            $("div.message").addClass("message alert alert-warning");
            //throw new Error(message);
        }

        const folders = await response.json();
        debugger
        if (folders.length > 0) {
            folders.forEach((data) => {
                createFileElement(data)
            })

            createFileEventListener();
        }

        document.getElementById('spinner').style.display = 'none'
    }

    function createFolderEventListener() {
        const myfolder = document.getElementsByClassName("folder");
        [...myfolder].forEach(function (element) {
            element.addEventListener("click", function () {
                fetchFolder(element.dataset.id);
                fetchFiles(element.dataset.id);
            });
        });
    };

    function createFileEventListener() {
        const myfolder = document.getElementsByClassName("file");
        [...myfolder].forEach(function (element) {
            element.addEventListener("click", function () {
                downloadPdf(element.dataset.id, element.textContent)
            });
        });
    };

    $(function () {
        document.getElementById('spinner').style.display = 'none'
        fetchFolder(1);
    })

    div.folder {
        border: 1px solid black;
        margin: 2px;
        cursor: pointer
    }

    div.file {
        border: 1px solid black;
        margin: 2px;
        cursor: pointer
    }

<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<div class="message"></div>
<img src="~/img/loading.gif" id="spinner" />
<div id="rootFolder"></div>
<div id="files"></div>

Answer 1

Prefer modern syntax You're using a few ES2015+ features already, including async/await. If you're going to write a script containing modern syntax (which you should - it can make code much easier to read and write), best to use it everywhere. In particular:

• Use const (or let when you need to reassign) instead of var. var has too many problems to be worth using nowadays, such as an unintuitive function scope (instead of block scope), hoisting, and implicitly creating properties on the global object when on the top level. (Use the no-var linting rule.)

• In ES2015+, iterators are available. Utilizing these can make code significantly more concise. For example:

  const myfolder = document.getElementsByClassName("folder");
  [...myfolder].forEach(function(element) {
  

  can be

  for (const element of document.querySelectorAll('.folder')) {
  

jQuery or not? You're using a bit of jQuery, and a lot of vanilla JS DOM manipulation. This is odd, stylistically. One would usually expect, if jQuery is used, for it to be used everywhere that it can be, at least regarding the DOM. But jQuery doesn't really provide anything that can't be done just as easily in vanilla JS, so you could remove jQuery as a dependency entirely too, if you wished. (That's what I'd choose, but it's up to you.)

Attributes 2 things:

• When you want to assign to a data- attribute, consider using dataset instead of the more verbose setAttribute:

  li.setAttribute('data-id', data.filePathId);
  

  can be

  li.dataset.id =  data.filePathId;
  

• Classes can be also be set with dot notation, with className:

  li.setAttribute('class', "folder");
  

  can be

  li.className = 'folder';
  

Avoid direct insertion of HTML markup You have:

$("div.message").text(message);

That's a good way to do things - assign text to the text content of an element. But elsewhere, you have

li.innerHTML = data.folderName

That can result in arbitrary code execution. It can also result in formatting problems relating to HTML entities. It sounds very likely that you're expecting folderName to only contain a plain non-HTML string, in which case you should set only the text of the <li>:

li.textContent = data.folderName;

Even though the folder name from the API response is almost certainly trustworthy, using textContent instead of innerHTML is a good habit to get into.

Semicolons Sometimes you're using semicolons when they're needed, and sometimes you aren't. To be stylistically consistent, best to choose one style - either use them wherever appropriate, or don't use them at all. Note that if you don't use them at all, unless you're an expert, you may well eventually run into problems due to Automatic Semicolon Insertion. (I'd recommend using semicolons.) To enforce a particular style on your code, use a linter.

fetchFolder and fetchFiles errors If either the fetchFolder or fetchFiles request fails, there are 2 problems:

• You don't return or throw if the response is not OK. As a result, the await response.json(); will throw. The control flow is odd.
• You show the spinner at the beginning of the request, but if there's an error, the code will never get to the end of the function where the spinner is hidden again.

The two functions also do very similar things - they show the spinner, do a fetch, and on error, show an error element to the user. Make things more DRY by creating a wrapper function around them.

Or, you could also consider using Promise.all - wait for both requests to complete before updating the UI, to make things look more natural to the user.

const fetchPath = (path) => {
    const response = await fetch(`https://localhost:44371/api/${path}`);
    if (!response.ok) {
        throw new Error(response.status);
    }
    return response.json();
};
const fetchFolderAndFiles = (id) => {
    const spinner = document.getElementById('spinner');
    spinner.style.display = 'block';
    divFolder.textContent = '';
    await Promise.all([
        fetchPath(`files/${id}`),
        fetchPath(`folder/${id}`),
    ])
        .then(([files, folders]) => {
            showFiles(files);
            showFolders(folders);
        })
        .catch((error) => {
            const message = `An error has occured: ${error.message}`;
            const messageElm = document.querySelector('div.message');
            messageElm.textContent = message;
            messageElm.className = 'message alert alert-warning';
        })
        .finally(() => {
            spinner.style.display = 'block';
        });
};

Don't check array length before iterating There's no need for

if (folders.length > 0) {
    folders.forEach((data) => {

If the array is empty, no iterations will occur anyway. Surrounding the forEach in another if statement increases indentation unnecessarily and makes more unnecessary cognitive overhead.

Duplicate listener bug Every time fetchFolder is called, you also call createFolderEventListener, which does:

const myfolder = document.getElementsByClassName("folder");
[...myfolder].forEach(function (element) {
    element.addEventListener("click", function () {

So every time a folder is clicked, you're adding additional listeners to every folder element on the page. This will result in many unnecessary requests, and possibly more strange behavior.

Only add a listener to a folder if the folder doesn't already have a listener. Or, even better, add only a single listener, to a parent element, and utilize event delegation:

document.addEventListener('click', (e) => {
    if (!e.target.matches('.folder')) {
        return;
    }
    const { id } = e.target.dataset;
    fetchFolder(
        fetchFiles(id);
    );
});

Run that once, on pageload, and then you can remove createFolderEventListener.

Do the same thing for createFileEventListener, which has the same problem.

Precise variable names You have:

var divFiles = document.getElementById("files");

and

function createFileEventListener() {
  const myfolder = document.getElementsByClassName("file");

(createFileEventListener should probably be removed entirely, but if it isn't, make sure to change myfolder to myfile)

Precise variable names make code easier to read and helps prevent bugs. A variable name should probably only be plural if it refers to a collection. Otherwise, it should be singular. Similarly, a collection should have a plural variable name. Maybe call it filesContainer instead of divFiles, and fileDivs instead of myfolder.