I asked a question about what is better to use: simple code or modules when writing a simple script for a page. On the toaster answered that the modules, but my module is terrible.

About the modules most liked the description here .

Question: what is wrong with my module?

An example for a simple accordion:

"use strict"; $(document).ready(function(){ if($('.accordion').length){ accordion.init(); } }); var accordion = (function(){ var _openContent = function(el){ var wrap = el.closest('.accordion'), item = el.closest('.accordion__item'), items = wrap.find('.accordion__item'), content = item.find('.accordion__content'), contents = wrap.find('.accordion__content'), dur = 500; if(!item.hasClass('on')){ items.siblings().removeClass('on'); item.addClass('on'); contents.stop(true,true).slideUp(dur); content.stop(true,true).slideDown(dur); } else { item.removeClass('on'); content.stop(true,true).slideUp(dur); } } // Инициализация return { init: function(){ $('.accordion__trigger').on('click', function(e){ e.preventDefault(); var $this = $(this); _openContent($this); }); } } })(); 
 *, *:before, *:after { box-sizing: border-box; } html, body { margin: 0; padding: 0; } a { text-decoration: none; color: inherit; transition: all .27s ease-in-out; } a:hover { color: tomato; } ul { padding: 0; margin: 0; list-style-type: none; } body { font-family: 'Raleway', sans-serif; font-size: 1rem; line-height: 1.5rem; color: #727272; } .accordion { width: 100%; margin: 2rem auto; padding: 0 2rem; } .accordion__item { box-shadow: 0 2px 7px rgba(0,0,0,0.17); } .accordion__title { text-transform: uppercase; margin: 0; } .accordion__trigger { display: block; color: #fff; padding: .5rem 1rem; background: #9c27b0; box-shadow: 0 2px 7px rgba(0,0,0,0.27); padding-left: 3.5rem; position: relative; } .accordion__trigger:before { content: '+'; position: absolute; left: 0; top: 0; bottom: 0; width: 2rem; text-align: center; line-height: 2.5rem; background: #ff4051; font-weight: 800; transition: all .34s ease-in-out; } .on .accordion__trigger:before { content: '-'; } .accordion__trigger:after { content: ' '; position: absolute; left: 2rem; top: 0; bottom: 0; width: 0; height: 0; border-top: 1.25rem solid transparent; border-bottom: 1.25rem solid transparent; border-left: .5rem solid #ff4051; } .accordion__trigger:hover { color: #fff; background: #7b1fa2; } .accordion__content { padding: 1.5rem 3.5rem; display: none; } .accordion__content span { display: block; text-transform: uppercase; color: #212121; font-weight: 700; margin-bottom: 1rem; } 
 <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script> <div class="accordion"> <ul class="accordion__list"> <li class="accordion__item"> <a href="#" class="accordion__trigger"> <h5 class="accordion__title">Lorem ipsum 1.</h5> </a> <div class="accordion__content"> <p> <span>Content 1. </span> Lorem ipsum dolor sit amet, consectetur adipisicing elit. Inventore saepe nulla iure cupiditate libero accusantium deleniti in, quisquam quae quidem. </p> </div> </li> <li class="accordion__item"> <a href="#" class="accordion__trigger"> <h5 class="accordion__title">Lorem ipsum 2.</h5> </a> <div class="accordion__content"> <p> <span>Content 2. </span> Lorem ipsum dolor sit amet, consectetur adipisicing elit. Inventore saepe nulla iure cupiditate libero accusantium deleniti in, quisquam quae quidem. </p> </div> </li> <li class="accordion__item"> <a href="#" class="accordion__trigger"> <h5 class="accordion__title">Lorem ipsum 3.</h5> </a> <div class="accordion__content"> <p> <span>Content 3. </span> Lorem ipsum dolor sit amet, consectetur adipisicing elit. Inventore saepe nulla iure cupiditate libero accusantium deleniti in, quisquam quae quidem. </p> </div> </li> </ul> </div>

  • I would not use a terrible word. It is a good code. But a toaster - this is still the ghetto IMHO. - user207618 pm
  • Offtopic: did they come up with the styles themselves or did they take them somewhere? - user207618 pm
  • one
    One thing I don’t really like is that there are too many calls to the DOM. Placing an accordion module is normal, someone generally advocates that the code before the absurd should be as modular as possible. But still need to know when to stop. Here you can. - user207618
  • one
    @Other, jsbin.com/wetiso/edit?css,output =)) - HamSter
  • one
    Super! My thanks to you are flying OSI methods! Very well, I will add you to the authors of the table on the right. - user207618

1 answer 1

A couple of small notes:

  1. Use one style for quotes (either single or double). "use strict"; in your double, and the rest of the places - single
  2. For ; choose one style too. If you put it everywhere, then you have missed after closing the function ( Function Expression ) var _openContent = function(el){ /* ... */ }; and then return { /* ... */ };

  3. You use $ in the module - an external library, it is better to transfer it to the module itself as a parameter:

    var accordion = (function($){ /* ... */ })($);

Immediately it is clear that to use your library you need a jQuery library. When using other dependent libraries, they can also be passed as a parameter to the module .

  1. "use strict"; it is better to transfer directly to the module , so that it would apply to it var accordion = (function($){ 'use strict'; /* ... */ })($);

  2. I think it would be nice if you could transfer parameters ( wrap , item , dur , etc.) to the module. And you should leave the values ​​that you have now by default. It would look something like this:

 // в init передаем объект с параметрами accordion.init({ dur: 1000 }); // ... _openContent($this, setting); // передаем эти параметры в _openContent // ... var _openContent = function(el, setting){ setting = setting || {}; // по умолчанию пустой объект var dur = setting.dur || 500; // -> 1000 var wrapSelector = setting.wrapSelector ||'.accordion'; // -> '.accordion'
  1. Recommendation (like all other points), but here is a matter of taste. If the variable value is a jQuery object , then the variable name can be started with $ - var $container = $('.container') . Then the code is immediately clear (for example, by your example, here: contents.stop(true,true) ) that we are working in this place with a jQuery object , and not with a regular js object that has a stop method. In the process of writing code, refactoring or searching for errors, it becomes clear what we are working with (perhaps not immediately, but I think you understand what I mean)

For the first two points, I recommend using eslint (or other code parsers) that will immediately emphasize (output) such errors.

And if you have not yet formed your own style of writing code, or when working in a team, it is convenient to use a single style, for example, Google JavaScript Style Guide or StandartJS (for which you do not need to put a semicolon). I myself adhere to the Airbnb JavaScript Style Guide ( in Russian ) - it is very well written.