How can I make my JS code better?

I have finished my final project for CS50 a while ago. However, since finishing it I have been considering refactoring my JS code, and it’s been bugging me since then, but I’m not sure in what way I can make it better (both design-wise and performance-wise).

Here’s a link to the code in question on GitHub: https://github.com/AmirF27/tracklog/blob/master/static/js/script.js
Hoping to get some advice from members more experienced than I am, although obviously I would be grateful for any suggestions.

In case you wish to check out my final project, here’s a link: https://tracklog-app.herokuapp.com/
And by the way, if you have any feedback I would appreciate it. :slight_smile:

Why are all platforms not available as options initially? Was the list not available because of technical performance or better user experience?
The auto-complete for the game options was a helpful feature. I liked that you gave a warning to users that deleting a platform could cause them to lose data. Making the modal header red could call more attention to the user. Look at Github ui when you try to delete a repo. They use color and text to warn the user.
Good job on your project. :slight_smile:

First and foremost, great work! I’m sure you put a lot of time and thought into this and it really shows. Excellent.

For a small project, with limited functionality. Particularly as an experiment or Proof-of-concept, it’s probably not worth changing much. However, if this were the beginnings of an application that you intended to expand upon over time, then I would make the following changes:

  • Use more specific function and variable names.
    This will make your code easier to read and will cut down on the number of comments you need.

  • Break your functions down into multiple smaller functions.
    To quote Bob Martin (author of Clean Code).

“The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that”

When you find yourself adding comments to describe what different parts of a function do, this is a signal that you should be breaking up the code. For example, I would refactor the following:

function createListItem(game) {
    // if no results were found, return a list item specifying as much
    if (game === undefined) {
        return $("<li/>", {
            class: "list-group-item text-center",
            text: "No games were found matching your input."
        });
    }

    // create the list item
    var listItem = $("<li/>", {
        class: "list-group-item result-item",
        "data-game-id": game.id,
    });

    // some game covers were missing from API, so had to check
    // if it's missing in order to display a placeholder instead
    var img;
    if (game.cover) {
        img = game.cover.url;
    }
    else {
        img = "https://placeholdit.imgix.net/~text?txtsize=22&txt=Missing%20cover&w=90&h=90";
    }

    // create a wrapper for list content and append game data to it
    var wrapper = $("<div/>", {
        class: "list-content-wrapper"
    });
    $("<img/>", {
        src: img,
        alt: game.name
    }).appendTo(wrapper);
    wrapper.append(game.name);

    // append wrapper with game data to the created list item
    wrapper.appendTo(listItem);

    return listItem;
}

into:

function createListItemFor(game) {
    if (game === undefined) {
        return createGameNotFoundListItem();
    }
    var listItemForGame = createGameListItem(game.id);
    var gameCoverImage = getGameCoverImg(game.cover);
    var wrapper = createListContentWrapper();
    var wrapperWithGameData = appendGameDataToWrapper(wrapper, game.name, gameCoverImage);
    var listItemWithWrapper = appendWrapperToListItem(listItemForGame, wrapperWithGameData);
    return listItemWithWrapper;
}

This type of refactoring makes your code easier to read, test, and reuse.

  • Assign variables for your icon classes
var expandIcon = "fa-plus-square-o"
var collapseIcon = "fa-minus-square-o"

This allows you to change the design without having to perform any big search and replace operations.

  • Put your modules in separate files and use a bundler like webpack
    (again, only worth it if you treat this code as the starting point for a larger project).

That’s my take, based on a quick look. Hope it’s helpful. Like I said though, overall, great work.

Feel free to check out my other advice on the Free Code Camp blog at:
https://medium.freecodecamp.com/@BillSourour

2 Likes

The best thing you can do is check for you code again and try find errors before they come. Try considering every option available. that way you’ll get a better code. If you’re having problems doing that alone you can try and get some help from a program. I tend to use checkmarx for code verification.
Good luck!
Michael.

@BillSourour Yes and another way might be to declare a game object and place all functionality in it to make it more obvious.

var game = {
id : 0,
cover : 'path_to_image",
createGameListItem : function(this.id){
// some code
},
getGameCoverImg : function(this.cover){
}
};

That way you encapsulate all properties and functionst into one plain old js object and therefore have all in one place.

var list = game.createGametListItem(game.id);

would be more compact solution.

Yes. That is an option, so long as we keep our createListContentWrapper() and appendWrapperToListItem() functions outside of the game Object, because I don’t think they are specific to game.

It’s also worth noting that my example breaks the Law of Demeter. If we want to be purists, and comply, it should be:

function createListItemFor(id, name, cover) {
    if (id === undefined) {
        return createGameNotFoundListItem();
    }
    var listItemForGame = createGameListItem(id);
    var gameCoverImage = getGameCoverImg(cover);
    var wrapper = createListContentWrapper();
    var wrapperWithGameData = appendGameDataToWrapper(wrapper, name, gameCoverImage);
    var listItemWithWrapper = appendWrapperToListItem(listItemForGame, wrapperWithGameData);
    return listItemWithWrapper;
}

As Martin Fowler points out though, it’s sometimes worth regarding the Law of Demeter as the Ocassionally Useful Suggestion of Demeter. I’m not convinced that given the scope of our domain model here, strict adherence is warranted, but if the project got big enough and we started adding more domains, it would probably be helpful.

EDIT: This is where destructuring syntax in ES6 comes in handy. Because we can get the best of both worlds with a signature like:

function createListItemFor({ id, name, cover } = {}) {...}

I’m madly in love with that pattern at the moment, by the way.