4
\$\begingroup\$

I refactored the code for a library that creates toasts. I have borrowed most of my naming conventions (and constant naming conventions in specific) as well as the design pattern from Bootstrap's source code.

What could be done to improve the code's readability? How would it be more maintainable? What do you dislike about the current style? Which parts do you think to be unnecessary?

const Toast = (function(){
  
  const Classes = {
    CONTAINER: 'toastContainer',
    TOAST_NEUTRAL: 'toastNeutral',
    TOAST_SUCCESS: 'toastSuccess',
    TOAST_ERROR: 'toastError',
    TOAST_WARNING: 'toastWarning',
    HEADER: 'toastHeader',
    CONTENT: 'toastContent'
  };
    
  const Ids = {
    TOASTER_ELEMENT: 'toasterElement'
  };
  
  const ToastTypes = {
    NEUTRAL: {
      HEADER_MESSAGE: "Info:",
      CLASS_NAME: Classes.TOAST_NEUTRAL
    },
    SUCCESS: {
      HEADER_MESSAGE: 'Success!',
      CLASS_NAME: Classes.TOAST_SUCCESS
    },
    ERROR: {
      HEADER_MESSAGE: 'Error!',
      CLASS_NAME: Classes.TOAST_ERROR
    },
    WARNING: {
      HEADER_MESSAGE: 'Warning!',
      CLASS_NAME: Classes.TOAST_WARNING
    }
  };
  
  
  const Toast_Utils = (function(){
    const _generateDiv = function _generateDiv(content, ...classNames) {
      return $('<div></div').addClass(classNames.join(' ')).html(content);
    };
    
    const createToastContainer = function createToastContainer(toastType) {
      return _generateDiv(null, Classes.CONTAINER, ToastTypes[toastType].CLASS_NAME)
        .hide();
    };

    const createToastHeader = function createToastHeader(toastType) {
      return _generateDiv(ToastTypes[toastType].HEADER_MESSAGE, Classes.HEADER);
    };

    const createToastContent = function createToastContent(toastMessage) {
      return _generateDiv(toastMessage, Classes.CONTENT);
    };
    
    return {
      createToastContainer: createToastContainer,
      createToastHeader: createToastHeader,
      createToastContent: createToastContent
    };
  })();
  
  const Toast = function Toast(toastType, toastMessage) {
    if(!Toast.toasterElement) {
      throw new Error('You have to call "Toast.init()" before creating "Toast" instances!');
    }
    if(!typeof toastType == 'string' || !ToastTypes[toastType.toUpperCase()]) {
      const acceptedToastTypes = Object.keys(ToastTypes).map(function(el){
        return '"' + el.toLowerCase() + '"';
      }).join(',');
      throw new Error('You must pass one of these strings for the first parameter: ' + acceptedToastTypes);
    }
    toastType = toastType.toUpperCase();
    arguments.length < 2 && (toastMessage = "");
    this.toast = Toast_Utils.createToastContainer(toastType)
      .append(Toast_Utils.createToastHeader(toastType))
      .append(Toast_Utils.createToastContent(toastMessage));
  };
  
  Toast.init = function() {
    Object.defineProperty(this, 'toasterElement', {
      value: $('<div></div>')
        .attr('id', Ids.TOASTER_ELEMENT),
      writable: false,
      configurable: false
    });
    $(document.body).append(this.toasterElement);
    return this.toasterElement;
  };
  
  Toast.prototype.begin = function() {
    Toast.toasterElement.append(this.toast);
    this.toast.fadeIn(500);
  };
    
  Toast.prototype.destroy = function destroy() {
    this.toast.fadeOut(500, () => {
        this.toast.remove();
    });
  };
  
  return Toast;
})();



$(document).ready(function() {

  Toast.init();
  
  $(document).on("click", "#btnNeutral", function() {
    let toast = new Toast("neutral", "Click on the bottom-most icon to change the website theme");
    toast.begin();
    setTimeout(toast.destroy.bind(toast), 3000);
  });
  
  $(document).on("click", "#btnSuccess", function() {
    let toast = new Toast("success", "everything went alright");
    toast.begin();
    setTimeout(toast.destroy.bind(toast), 3000);
  });

  $(document).on("click", "#btnError", function() {
    let toast = new Toast("error", "something went wrong");
    toast.begin();
    setTimeout(toast.destroy.bind(toast), 3000);
  });
  
  $(document).on("click", "#btnWarning", function() {
    let toast = new Toast("warning", "a bomb went off 24km from your location");
    toast.begin();
    setTimeout(toast.destroy.bind(toast), 3000);
  });
  
});
#toasterElement {
  position: absolute;
  top: 0;
  right: 0;
  z-index: 1;
  width: 20%;
}

.toastContainer {
  margin-top: 10px;
  font-weight: bold;
  font-family: arial;
  width: 100%;
  padding: 10px;
  color: #ffffff;
  border-left: 5px solid white;
  box-shadow: -2px 0 2px rgba(0, 0, 0, .2);
  position: relative;
  z-index: 1;
}

.toastNeutral {
  background-color: #242424;
}

.toastSuccess {
  background-color: #32cd32;
}

.toastError {
  background-color: #ff1a1a;
}

.toastWarning {
  background-color: #fcdc47
}

.toastContent {
  margin-top: 5px;
  font-size: 70%;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>

<button id="btnNeutral">
Neutral
</button>

<button id="btnSuccess">
Success
</button>

<button id="btnError">
Error
</button>

<button id="btnWarning">
Warning
</button>

\$\endgroup\$
2
\$\begingroup\$

Small typo, and a simplification

Here's a small typo:

return $('<div></div').addClass(classNames.join(' ')).html(content);

And a slightly simpler way to write that:

return $('<div/>').addClass(classNames.join(' ')).html(content);

Naming

In Toast_Utils, the name of the object already implies that they are related to "toast", no need to repeat the word "toast" in the functions. For example instead of createToastContainer, createContainer should be clear enough, as it will be used as Toast_Utils.createContainer.

Also, I would inline the definition of all these functions to reduce the boilerplate code.

Arrow functions

You mostly use arrow functions, except in a few places, for example in the map call here:

  const acceptedToastTypes = Object.keys(ToastTypes).map(function(el){
    return '"' + el.toLowerCase() + '"';
  }).join(',');

And as a tiny usability improvement, I would add a space after the comma when joining, to make the error message slightly easier to read.

Readability

A common convention is to put spaces between if and the (...) condition, for example in if(!Toast.toasterElement) {.


Is this a common writing style?

arguments.length < 2 && (toastMessage = "");

I would spell it out as an if statement.

\$\endgroup\$
  • \$\begingroup\$ 1. Thanks! Never knew you could use that shorthand. 2. Agree 3. Actually, I rarely use arrow functions (I am not really ES6 literate right now), it was just that I needed to refer to this inside of the callbacks and I wanted to avoid using the var self = this..... pattern. 4. Thanks! 5. Are you saying I should comment that out ? I am not using a ternary expression because that would require an extra null for cases where arguments.length >= 2, I wanted it to be as concise as possible. Isn't that a good thing ? \$\endgroup\$ – doubleOrt Jan 1 '18 at 16:35
  • \$\begingroup\$ For #5, everything would be alright without that check, JQuery's .html() would simply ignore undefined. But, I added it so that the library is not dependent on JQuery's functionality, because let's say JQuery decides that they no longer want their .html() method to ignore undefined, there would be a bug in my library immediately (where "undefined" would be printed out as the toast message even when the user does not pass anything for that parameter). \$\endgroup\$ – doubleOrt Jan 1 '18 at 16:40
  • \$\begingroup\$ Do you agree with the necessity or the usage of the Toast_utils object? I didn't want to expose anything unnecessary to the outer scope (Principle of Least Exposure ?). \$\endgroup\$ – doubleOrt Jan 1 '18 at 16:43
  • 1
    \$\begingroup\$ @Taurus in Toast_Utils, I like that it hides the common utility function _generateDiv. But I think it's overkill. The closure of Toast already hides most implementation details. So Toast_Utils is essentially private implementation within private implementation. Strictly speaking it's correct to hide _generateDiv, but it's unimportant. \$\endgroup\$ – janos Jan 1 '18 at 17:46
  • 1
    \$\begingroup\$ @Taurus I'm not sure what you mean by "underscore before Toast_Utils". If you mean _Toast_Utils, I don't see a good reason for such naming convention. \$\endgroup\$ – janos Jan 1 '18 at 21:47
1
\$\begingroup\$

I like the pattern of defining the used CSS IDs and classes at the very beginning.

I don't see the need for a separate utility scope. I would just write these functions inline, or inline their code as well.

You can replace the error handling by making the public API so simple that it cannot be used incorrectly anymore. The typical use case is not to create a toast for later reuse, but instead to show it immediately. The API should reflect this.

Making the user of the module call init explicitly is not necessary. The toast container should be initialized when the first toast is to be shown.

I prefer a JavaScript style that doesn't refer to this or prototypes. It looks cleaner and doesn't require as much thinking about which objects are bound to which functions.

Using these guidelines, I transformed your code to this:

const Toast = (function () {

    const Ids = {
        TOASTER: 'toasterElement'
    };

    const Classes = {
        CONTAINER: 'toastContainer',
        TOAST_NEUTRAL: 'toastNeutral',
        TOAST_SUCCESS: 'toastSuccess',
        TOAST_ERROR: 'toastError',
        TOAST_WARNING: 'toastWarning',
        HEADER: 'toastHeader',
        CONTENT: 'toastContent'
    };

    let toaster = null;

    function show(cssClass, heading, text, duration) {
        if (toaster === null) {
            toaster = $('<div>').attr('id', Ids.TOASTER);
            $(document.body).append(toaster);
        }

        const header = $('<div>').addClass(Classes.HEADER).text(heading);
        const content = $('<div>').addClass(Classes.CONTENT).text(text);
        const container = $('<div>').addClass(Classes.CONTAINER).addClass(cssClass).hide();
        const toast = container.append(header).append(content);
        toaster.append(toast);

        function destroy() {
            toast.fadeOut(500, () => toast.remove());
        }

        toast.fadeIn(500, () => setTimeout(destroy, duration));
    }

    function showNeutral(text, duration) {
        show(Classes.TOAST_NEUTRAL, 'Info:', text, duration);
    }

    function showSuccess(text, duration) {
        show(Classes.TOAST_SUCCESS, 'Success!', text, duration);
    }

    function showWarning(text, duration) {
        show(Classes.TOAST_WARNING, 'Warning!', text, duration);
    }

    function showError(text, duration) {
        show(Classes.TOAST_ERROR, 'Error!', text, duration);
    }

    return {
        showNeutral: showNeutral,
        showSuccess: showSuccess,
        showWarning: showWarning,
        showError: showError,
    };
})();

$(document).ready(function () {
    $(document).on("click", "#btnNeutral", () => {
        Toast.showNeutral("Click on the bottom-most icon to change the website theme", 3000);
    });
    $(document).on("click", "#btnSuccess", () => {
        Toast.showSuccess("everything went alright", 3000);
    });
    $(document).on("click", "#btnError", () => {
        Toast.showError("something went wrong", 3000);
    });
    $(document).on("click", "#btnWarning", () => {
        Toast.showWarning("a bomb went off 24km from your location", 3000);
    });
});
\$\endgroup\$
  • \$\begingroup\$ 1. Thanks 2. I do agree that the separate scope is unnecessary right now, but it could become necessary very quickly if I were to add one or two more functions like _generateDiv(). Same thing for the other functions in that scope, they are simple enough to be inline right now, but I could easily see them become more complex, in which case, you would have to separate them again anyways. 3. I don't know. init IS probably unnecessary, but I think destroy is necessary. I would want potential users of the library to hold as much power over the library as is possible. However, \$\endgroup\$ – doubleOrt Jan 1 '18 at 18:43
  • \$\begingroup\$ continuing... I think my approach is easier to update and it generalizes the implementations of the different toasts, for example, if I wanted to add in a "primary" toast type, all I would have to do is add the class to CLASSES and then add a PRIMARY object to the ToastTypes object. If I were to use functions for each toast type, then I would have to add a class to CLASSES and then create a new function for every single type of toast I ever wanted to add, and then return a reference to that in the final object. Aren't strings also easier than functions to use in conditions and such \$\endgroup\$ – doubleOrt Jan 1 '18 at 18:48
  • \$\begingroup\$ continuing... , for example: a == false ? Toast.showSuccess(msg, 1000) : Toast.showError(msg , 1000); Vs new Toast(a == false ? "success" : "error", msg) (of course things would get more complicated with my solution as well if I were to generate a message conditionally and not use a variable, I hope you get my point though). 4. I wholeheartedly agree, that init method was dumb. 5. Which pattern would you use if you wanted to make things object-oriented ? I also like the duration parameter you have defined (I would like to disallow anything other than an integer value tho) \$\endgroup\$ – doubleOrt Jan 1 '18 at 18:59
  • 1
    \$\begingroup\$ I agree that having the header messages all defined in one place at the top of the file is good. For all the other points you mentioned: You aren't gonna need them. Most probably. Therefore it is moot to think about extension points. Better wait until someone other than you wants a particular feature. \$\endgroup\$ – Roland Illig Jan 4 '18 at 0:23
  • 1
    \$\begingroup\$ And no, object-oriented is not good in this case. If this simple API covers all use cases, having these new fixed functions is great. Adding object orientation and classes will only make the code harder to read. \$\endgroup\$ – Roland Illig Jan 4 '18 at 0:24

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.