2
\$\begingroup\$

I have the following code in jsfiddle. I have been told this this code is messy and not optimised. My question is, what about this is messy and unoptimised and how can it be tidied up and optimised to reduce file size and so it becomes more responsive?

$(document).ready(function() {

    $( ".column" ).sortable({
        connectWith: ".column",
        handle: ".widget_header_icon, .widget_header_title",
        start: function( event, ui ) {
            $('.menu_button.active, .configure_button.active').click();
            $('.menu_button, .configure_button').removeClass("active");
        }
    });

    $(document).click(function(event) {
        $('.menu_button.active, .configure_button.active').click();
        $('.menu_button, .configure_button').removeClass("active");
    });

    $('.dropdown_left, dropdown_right').each(function() {
        $(this).css('left', $(this).prev().position().left);
    });

    $('.menu_button, .configure_button').click(function(event) {
        $(this).siblings('.menu_button.active, .configure_button.active').click();
        $(this).toggleClass('active').next().toggle();
        event.stopPropagation();
    });

    $('.widget_configure_button').click(function(event) {
        var $nav3 = $(this),
            $dd = $nav3.next('.dropdown');

        $nav3.toggleClass('active');

        $dd.css({
            top: $nav3.outerHeight()+10,
            right: 10
        });

        $nav3.hasClass('active') ? $dd.show() : $dd.hide();
    });

});
/* http://meyerweb.com/eric/tools/css/reset/ 
   v2.0 | 20110126
   License: none (public domain)
*/

html, body, div, span, applet, object, iframe,
h1, h2, h3, h4, h5, h6, p, blockquote, pre,
a, abbr, acronym, address, big, cite, code,
del, dfn, em, img, ins, kbd, q, s, samp,
small, strike, strong, sub, sup, tt, var,
b, u, i, center,
dl, dt, dd, ol, ul, li,
fieldset, form, label, legend,
table, caption, tbody, tfoot, thead, tr, th, td,
article, aside, canvas, details, embed, 
figure, figcaption, footer, header, hgroup, 
menu, nav, output, ruby, section, summary,
time, mark, audio, video {
    margin: 0;
    padding: 0;
    border: 0;
    font-size: 100%;
    font: inherit;
    vertical-align: baseline;
    font-family:arial;
    font-size:12px;
}
/* HTML5 display-role reset for older browsers */
article, aside, details, figcaption, figure, 
footer, header, hgroup, menu, nav, section {
    display: block;
}
body {
    line-height: 1;
}
ol, ul {
    list-style: none;
}
blockquote, q {
    quotes: none;
}
blockquote:before, blockquote:after,
q:before, q:after {
    content: '';
    content: none;
}
table {
    border-collapse: collapse;
    border-spacing: 0;
}

/**/

body
{
    background:#F5F5F5;
}

.notification
{
    background:#FFFFFF;
}

.inner_notification
{
    width:983px; /*1003 max width without horizontal scroll for 1024x768 screens*/
    margin:auto;
    padding:10px;
    background:#EEEEEE;
}

.header
{
    background:#EEEEEE;
}

.inner_header
{
    width:983px; /*1003 max width without horizontal scroll for 1024x768 screens*/
    margin:auto;
    padding:10px;
    background:#DDDDDD;
}

.top_menu
{
    background:#DDDDDD;
}

.inner_top_menu
{
    width:1003px; /*1003 max width without horizontal scroll for 1024x768 screens*/
    margin:auto;
    position: relative;
    background:#CCCCCC;
}

.menu_button {
    float:left;
    padding:10px;
    cursor:pointer;
}

.configure_button {
    float:right;
    padding:10px;
    cursor:pointer;
}

.widget_configure_button {
    float:right;
    padding:10px;
    cursor:pointer;
}

.menu_button::selection, .configure_button::selection, .widget_configure_button::selection { background:transparent; }
.menu_button::-moz-selection, .configure_button::-moz-selection, .widget_configure_button::-moz-selection { background:transparent; }

.menu_button:hover, .configure_button:hover, .widget_configure_button:hover {
    background-color:#ffffff;
}

.menu_button.active, .configure_button.active, .widget_configure_button.active  {
    padding:10px;
    background-color:#ffffff;
    z-index:1;
}

.dropdown 
{
    display:none;
    background-color:#ffffff;
    position:absolute;
    top:40px;
    padding:10px;
    cursor:pointer;
}

.dropdown_left {
    display:none;
    background-color:#ffffff;
    position:absolute;
    top:32px;
    padding:10px;
    cursor:pointer;
    z-index:20;
}

.dropdown_right 
{
    left: auto ! important;
    right: 0;
}

.clearfix
{
    clear:both;
}

.title
{
    background:#CCCCCC;
}

.inner_title
{
    width:983px; /*1003 max width without horizontal scroll for 1024x768 screens*/
    margin:auto;
    padding:10px;
    background:#BBBBBB;
}

.content
{
    background:#BBBBBB;
}

.inner_content
{
    width:993px; /*1003 max width without horizontal scroll for 1024x768 screens*/
    margin:auto;
    padding:10px 0px 0px 10px;
    background:#AAAAAA;
}

.column
{
    width:331px;
    float:left;
}

.widget
{
    background:#EEEEEE;
    padding:10px;
    margin-bottom:10px;
    margin-right:10px;
    position:relative;
    z-index:10;
}

.widget_header
{
    background:#DDDDDD;
    margin-bottom:10px;
    overflow: hidden;
}

.widget_header_icon
{
    padding:10px;
    float:left;
    cursor:move;
}

.widget_header_title
{
    padding:10px;
    float:left;
    cursor:move;
    background:#CCCCCC;
    width:210px;
}

.widget_sub_header
{
    background:#DDDDDD;
    margin-bottom:10px;
    padding:10px;
    overflow: hidden;
}

.widget_content
{
    background:#CCCCCC;
    padding:10px;
    margin-bottom:10px;
    height:200px;
}

.widget_footer
{
    background:#BBBBBB;
    padding:10px;
}

.footer
{
    background:#AAAAAA;
}

.inner_footer
{
    width:983px; /*1003 max width without horizontal scroll for 1024x768 screens*/
    margin:auto;
    padding:10px;
    background:#999999;
}

/*-------------*/

.ui-sortable-placeholder { background:#BBBBBB; visibility: visible !important; }
.ui-sortable-placeholder * { visibility: hidden; }

/*------------*/
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">

<html>
    <head> 
        <meta http-equiv="content-type" content="text/html; charset=UTF-8">

        <title></title> 
        <script type="text/javascript" src="jquery.min.js"></script>
        <script type="text/javascript" src="jquery-ui.min.js"></script>
        <script type="text/javascript" src="script.js"></script>

        <link type="text/css" href="style.css" rel="stylesheet" />
    </head> 
    <body>
        <div class="notification">

            <div class="inner_notification">
                notification
            </div>

        </div>

        <div class="header">

            <div class="inner_header">
                header
            </div>

        </div>

        <div class="top_menu">

            <div class="inner_top_menu">
                <div class="menu_button">menu</div>
                <div class="dropdown_left">
                    <div>icon Default 2</div>
                    <div>icon Reports 2</div>
                    <div>icon Other 2</div>
                </div>
                <div class="configure_button">(c)</div>
                <div class="dropdown_left dropdown_right">
                    <div>icon Default 2</div>
                    <div>icon Reports 2</div>
                    <div>icon Other 2</div>
                </div>

                <div class="clearfix"></div>

            </div>

        </div>

        <div class="title">

            <div class="inner_title">
                title
            </div>

        </div>

        <div class="content">

            <div class="inner_content">
                <div class="column">

                    <div class="widget">
                        <div class="widget_header">
                            <div class="widget_header_icon">( i )</div>
                            <div class="widget_header_title">header title</div>
                            <div class="widget_configure_button">(c)</div>
                            <div class="dropdown">
                                <div>icon Default 3</div>
                            </div>
                        </div>
                        <div class="widget_sub_header">
                            sub header
                        </div>
                        <div class="widget_content">
                            content
                        </div>
                        <div class="widget_footer">
                            footer
                        </div>
                    </div>

                    <div class="widget">
                        <div class="widget_header">
                            <div class="widget_header_icon">( i )</div>
                            <div class="widget_header_title">header title</div>
                            <div class="widget_configure_button">(c)</div>
                            <div class="dropdown">
                                <div>icon Default 3</div>
                            </div>
                        </div>
                        <div class="widget_sub_header">
                            sub header
                        </div>
                        <div class="widget_content">
                            content
                        </div>
                        <div class="widget_footer">
                            footer
                        </div>
                    </div>

                </div>

                <div class="column">
                    <div class="widget">
                        <div class="widget_header">
                            <div class="widget_header_icon">( i )</div>
                            <div class="widget_header_title">header title</div>
                            <div class="widget_configure_button">(c)</div>
                            <div class="dropdown">
                                <div>icon Default 3</div>
                            </div>
                        </div>
                        <div class="widget_sub_header">
                            sub header
                        </div>
                        <div class="widget_content">
                            content
                        </div>
                        <div class="widget_footer">
                            footer
                        </div>
                    </div>

                    <div class="widget">
                        <div class="widget_header">
                            <div class="widget_header_icon">( i )</div>
                            <div class="widget_header_title">header title</div>
                            <div class="widget_configure_button">(c)</div>
                            <div class="dropdown">
                                <div>icon Default 3</div>
                            </div>
                        </div>
                        <div class="widget_sub_header">
                            sub header
                        </div>
                        <div class="widget_content">
                            content
                        </div>
                        <div class="widget_footer">
                            footer
                        </div>
                    </div>
                </div>

                <div class="column">
                    <div class="widget">
                        <div class="widget_header">
                            <div class="widget_header_icon">( i )</div>
                            <div class="widget_header_title">header title</div>
                            <div class="widget_configure_button">(c)</div>
                            <div class="dropdown">
                                <div>icon Default 3</div>
                            </div>
                        </div>
                        <div class="widget_sub_header">
                            sub header
                        </div>
                        <div class="widget_content">
                            content
                        </div>
                        <div class="widget_footer">
                            footer
                        </div>
                    </div>

                    <div class="widget">
                        <div class="widget_header">
                            <div class="widget_header_icon">( i )</div>
                            <div class="widget_header_title">header title</div>
                            <div class="widget_configure_button">(c)</div>
                            <div class="dropdown">
                                <div>icon Default 3</div>
                            </div>
                        </div>
                        <div class="widget_sub_header">
                            sub header
                        </div>
                        <div class="widget_content">
                            content
                        </div>
                        <div class="widget_footer">
                            footer
                        </div>
                    </div>
                </div>

                <div class="clearfix"></div>
           </div>

        </div>

        <div class="footer">

            <div class="inner_footer">
                footer
            </div>

        </div>

    </body> 
</html>

Improve this question
\$\endgroup\$
  • 2
    \$\begingroup\$ There's a lot of code so I haven't gone through it all, but what strikes me is that you're not using classes very efficiently; you're namespacing everything, even when you don't need to. Instead of something like widget_header and widget_header_icon just call it header and icon. The css selector for the icon would then be .widget .header .icon \$\endgroup\$ – Flambino Nov 28 '12 at 18:37
  • \$\begingroup\$ @Flambino, I've already got a .header class for the header of the page. That is why I gave the header of the .widget's a different class. Because I want both headers to have different styled. Plus, I will have a general .icon class and .widget icon classes. \$\endgroup\$ – oshirowanen Nov 29 '12 at 10:05
  • 1
    \$\begingroup\$ I assume you are not interested in HTML (as you are only using div elements)? \$\endgroup\$ – unor Nov 29 '12 at 12:44
  • 3
    \$\begingroup\$ @oshirowanen You missed my point. Ok, you have an element with a header class already (in which case you should probably use the <header> element, or use an id attribute), but that's why CSS has selectors; a .widget .header selector will only style header-classed elements inside widget-classed elements. That's basic CSS. \$\endgroup\$ – Flambino Nov 30 '12 at 20:51
  • 1
    \$\begingroup\$ @Flambino: .widget .header has several problems. 1. If you have a nested component, for example '.notification .header' inside of the widget, the styles will interfere with each other. This becomes very problematic in large projects. 2. Quoting Mozilla: "The descendant selector is the most expensive selector in CSS. It is dreadfully expensive—especially if the selector is in the Tag or Universal Category." They recommend using .treecell-header {…} over treehead > treerow > treecell {…}. 3. Using simple selectors helps you avoid reliance on a specific DOM structure. \$\endgroup\$ – Alexey Lebedev Dec 7 '12 at 19:20
6
+25
\$\begingroup\$

Messy would imply that the code readability is bad, which is not the case. However...

  1. Your HTML is pretty much one big div. Semantics have value, so use proper elements for the adequate job. That's why we have semantic elements in the first place. Also, there's little reason to use anything else than the HTML5 doctype in this day and age. Overall, your absolute lack of HTML semantics is the greatest issue as it produces most collateral damage.

  2. Meyer's reset, while popular, is rather obsolete today and goes unnecessarily far (check out normalize.css, you might prefer it). While CSS can be bent in almost any way as long as you're confident in the outcome, I would say you lack experience to prototype a layout correctly. Far too many specific classes, the structure can be more generalized and shared. While I haven't tried your code, it appears to be very static (one dimension change requiring more dependant changes).

  3. Your JS searches the DOM for the same elements too much and on every function execution, this is unnecessary. Define their instances and reuse them.

If any of this is unclear, I'll be happy to go in further detail or expand the answer.

Improve this answer
\$\endgroup\$
2
\$\begingroup\$

  1. I'd create a Widget component. That would allow to create any number of Widgets without duplicating HTML, and all methods for widget manipulation would be grouped together.

    var widget = new Widget({
    header:  '...',
    content: '...'
});

  2. The variable names are not descriptive.

    $dd   -> $dropdownList
$nav3 -> $dropdownButton

  3. Selecting elements through .next() and .prev() is fragile, I'd select by class name instead.

    // in the constructor...
$dropdownList = $container.find('dropdown-list');
// in the event handler...
$dropdownList.toggle();

  4. You can save some JavaScript by structuring HTML and CSS differently. If you put the dropdown inside of the configure button, you wouldn't need to re-position it every time, or to hide it using JS. The event handler would be simplified to:

    $configureButton.click(function() {
    $(this).toggleClass('active');
});

  5. Selectors such as .title and .dropdown are overly general. As the project grows larger, dozens of different header and dropdown variants appear, and they start to clash with each other. By then refactoring becomes hard, and people start adding !important, which makes the situation even worse.

  6. event.stopPropagation() is usually really bad, it blocks events for other modules that need to hide their dropdowns, track clicks, or initialize some other behaviour.

Improve this answer
\$\endgroup\$
2
\$\begingroup\$

For someone to straight-up call this code messy is, I think, uncalled for.

I agree with mystrdat's answer suggesting the lack of semantics is a big deal. This is low-hanging fruit for 'optimizing' the markup. However, I think you have some larger issues.

The site is actually buggy.

When I click and drag the 'header title' elements onto one another the relayout is not what I would expect. It's not difficult to get them all in one column or moving around almost unpredictably. I'm almost certain this is not the intended behavior, which makes it a bug and a pretty big one. I think this is the biggest problem as it simply doesn't work. IMHO, this is far more significant than whether the code is messy or optimized.

When I tested in FF, the widget_header_title elements are too wide and this causes the widget_configure_button elements to be pushed down, making the widget_header elements to be twice as tall as you're going for (according to how it renders in Chrome). Changing the markup and CSS to the following fixes this, as far as I can tell:

HTML

<div class="widget_header">
    <div class="widget_header_icon">( i )</div>
    <div class="widget_configure_button">(c)</div>
    <div class="widget_header_title">header title</div> <!-- now 3rd item -->
    <div class="dropdown">
        <div>icon Default</div>
    </div>
</div>

CSS

/* taking out float and hard-coded width */
.widget_header_title {
    padding: 10px;
    cursor: move;
    background: #CCC;
    overflow: hidden;
}

However, this breaks breaks your dropdown code because the dropdown code relies on the .next() jQuery method. Relying on precise ordering of sibling elements in the DOM can yield brittle code. Maybe change this so the button and corresponding dropdown element are both inside a container element. This will be more stable, maintenance wise. An alternative is to add a data-target attribute (or something similar) on the button and set the value to a selector that will target the dropdown to toggle. The problem with this approach it generally requires the use of IDs.

Container:

<div class="dropdown_group">
    <div class="menu_button">menu</div>
    <div class="dropdown_left">
        <div>icon Default 2</div>
        <div>icon Reports 2</div>
        <div>icon Other 2</div>
    </div>
<div>

Related via data-target:

<div class="menu_button" data-target="#dropdown1">menu</div>
<div id="dropdown1" class="dropdown_left">
    <div>icon Default 2</div>
    <div>icon Reports 2</div>
    <div>icon Other 2</div>
</div>

Your JS relies a lot on dispatching UI events to trigger other code you've written. This should absolutely be avoided. It's difficult to read and is inefficient. If there is an operation that's executed by a click-handler that you need to execute, make it available as a function and call it. If that won't work or is not an option, then take a different approach.

Combining the two notes, your dropdown code could be written something like:

HTML

<div class="menu_wrap">
    <button class="menu_button">menu</button>
    <ul class="dropdown_left">
        <li><a href="#some_link">icon Default 2</a></li>
        <li><a href="#some_link">icon Reports 2</a></li>
        <li><a href="#some_link">icon Other 2</a></li>
    </ul>
</div>

CSS

.menu_wrap {
    position: relative;
}
.menu_wrap.open > .dropdown_left {
    display: block;
}

JS

$menu_wraps = $('.menu_wrap');

$menu_wraps.on('click.dropdownToggler', function(e){
    var $this = $(this);
    if ($this.hasClass('open')) {
        $this.removeClass('open');
    } else {
        $menu_wraps.filter('.open').removeClass('open');
        $this.addClass('open');
    }
    return false;
});

$(document).on('click.dropdownCloser', function(e) {
    $menu_wraps.filter('.open').removeClass('open');
});

Generally speaking, you may want to use CSS to position your dropdowns instead of JavaScript. Wrapping the dropdown elements in something like menu_wrap as noted above will make it possible to only use CSS for positioning the menus.

.menu_wrap {
    position: relative;
}
.dropdown_left {
    position: absolute;
    top: 100%;
    left: 10px;
    /* etc. */
}

In your CSS, there are a lot of situations where you have a hardcoded width. I try to avoid this because it makes for brittle layouts (like the widget_header_title elements). For example:

.inner_footer {
    width: 983px;
    margin: auto;
    padding: 10px;
    background: #999;
}

Can be written as:

.inner_footer {
    margin: 0 10px;
    padding: 10px;
    background: #999;
}

For this to be centered you need a containing element that has the width set, but this container can contain the rest of your site. This way, you only have the width set in one place:

HTML

<body>
    <div class="container">
        <section class="notification">
            <div class="inner_notification">
                notification
            </div>
        </section>

        <section class="header">
            <div class="inner_header">
                header
            </div>
        </section>

        <!-- etc -->

    </div><!-- .container -->
</body>

CSS

.container {
    width: 1003px;
    margin: 0 auto;
}

There are tradeoffs, as this doesn't have the various wide bands of color that fill the margins. For that, you would just want to add the container class to each of your ourtermost wrapping elements. But, you still have the width set in only one place in your CSS.

    <div class="notification container">
        <div class="inner_notification">
            notification
        </div>
    </div>

Per mystrdat's remark, those wrapping elements (.notification) should really be <section> elements. And, the menu triggers (configure_button, widget_configure_button and menu_button) should be <button> elements. Your dropdowns should be <ul> elements. Your footer could be a <footer> element. That would all make it more "semantic" which is definitely a good thing and generally helps when it comes to what other developers think of your markup (since it was mentioned in the OP).

Improve this answer
\$\endgroup\$

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.