4
\$\begingroup\$

I've written a web application that is a replica of the iPhone's notes app. It turned out quite well, you can do everything you can on the iPhone's app except for sharing and sending it via texts etc. I've been sitting here trying to figure out a way to DRY this up, so far I think I've done a pretty good job in doing so.

However, there is one thing that annoys me a bit and I haven't been able to come up with a better solution then what I have now. It's these parts:

ctrl.functions.hideBtn('#done-btn');
ctrl.functions.hideBtn('#all-notes');
ctrl.functions.hideBtn('#edit');
ctrl.functions.showBtn('#add-note');

I repeat myself a lot by doing these things.. And I do them like 4-5 times throughout the code whenever the view needs to change. What I tried to do in the showBtn and hideBtn is sending in an array containing all the buttons that needed to be manipulated and then using a loop in the respective functions to loop over that array and hide/show them. This, however, didn't work for some reason and after thinking about it it didn't really feel like a better solution, it just got more messy.

How can I remove this repetitiveness?

Also, feel free to leave feedback on anything else that can be improved. I'll leave the full code down below as well as two pictures of what it looks like for those who are interested.

$(document).ready(function() {

var model = {
    ajaxURLs: {
        addNote: '../includes/notepages/addnote.php',
        editNote: '../includes/notepages/editnote.php',
        deleteNote: '../includes/notepages/deletenote.php',
        allNotes: '../includes/notepages/allnotes.php',
    },
    wrappers: {
        topBar: $('#top-bar'),
        content: $('#content-wrapper'),
        timestamp: undefined
    }
}

var ctrl = {

    bindElements: (function() {
        model.wrappers.topBar.on('click', '#add-note', function() {

            var promise = ctrl.functions.restCall(model.ajaxURLs.addNote);
                promise.done(function(data) {

                    $('#add-note').hide();

                    ctrl.functions.showBtn('#all-notes');
                    ctrl.functions.showBtn('#done-btn');

                    ctrl.functions.appendData(data);
                });
        });

        model.wrappers.topBar.on('click', '#all-notes', function() {

            var allNotesPromise = ctrl.functions.restCall(model.ajaxURLs.allNotes);
                allNotesPromise.done(function(data) {

                    ctrl.functions.hideBtn('#done-btn');
                    ctrl.functions.hideBtn('#all-notes');
                    ctrl.functions.hideBtn('#edit');
                    ctrl.functions.showBtn('#add-note');

                    ctrl.functions.appendData(data);
                });
        });

        model.wrappers.topBar.on('click', '#done-btn', function() {

            var textarea = $('#notes-area');

            if (textarea.val().length === 0) {
                return false;
            }

            var allNotesPromise = ctrl.functions.restCall(model.ajaxURLs.allNotes);
                allNotesPromise.done(function(data) {

                    ctrl.functions.hideBtn('#done-btn');
                    ctrl.functions.hideBtn('#all-notes');
                    ctrl.functions.showBtn('#add-note');
                    ctrl.functions.appendData(data);
                });
        });

        model.wrappers.topBar.on('click', '#edit', function() {

            $('#notes-area').attr('disabled', false);

            ctrl.functions.hideBtn('#edit');
            ctrl.functions.showBtn('#done-btn');
        });

        model.wrappers.content.on('click', '#notes-list li', function() {

            model.timestamp = $(this).children('.timestamp').html();

            var editPromise = ctrl.functions.restCall(model.ajaxURLs.editNote, 'timestamp', model.timestamp);
                editPromise.done(function(data) {

                    ctrl.functions.appendData(data);
                    $('#timestamp-input').attr('value', model.timestamp);
                });

            ctrl.functions.hideBtn('#add-note');
            ctrl.functions.showBtn('#all-notes');
            ctrl.functions.showBtn('#edit');
        });

        model.wrappers.content.on('click', '#trashcan', function() {

            var deletePromise = ctrl.functions.restCall(model.ajaxURLs.deleteNote, 'timestamp', model.timestamp);
                deletePromise.done(function(data) {

                    var allNotesPromise = ctrl.functions.restCall(model.ajaxURLs.allNotes);
                        allNotesPromise.done(function(data) {

                            ctrl.functions.appendData(data);
                        });

                    ctrl.functions.hideBtn('#edit');
                    ctrl.functions.showBtn('#add-note');
                });
        });
    }()),
    functions: {

        init: function() {
            var promise = ctrl.functions.restCall('../includes/notepages/allnotes.php');
                promise.done(function(data) {
                    ctrl.functions.appendData(data);
                });
        },
        showBtn: function(id) {
            $(id).show();
        },
        hideBtn: function(id) {
            $(id).hide();
        },
        restCall: function(url, dataVar, data) {

            return $.ajax({
                url: url,
                method: 'post',
                data: '' + dataVar + '=' + data
            });
        },
        appendData: function(data) {
            model.wrappers.content.children().remove();
            model.wrappers.content.append(data).hide().fadeIn();
        }
    }
}

    window.onload = ctrl.functions.init;
});

This is the static HTML of the page, containing all the buttons (which is the main focus of this question):

<main class="full-height">
    <div id="top-bar">
        <button id="all-notes" class="btn brown-btn hide">Alla anteckningar</button>
        <button id="add-note" class="btn brown-btn">Ny</button>
        <input type="submit" form="note-text" value="Klar" id="done-btn" class="btn brown-btn hide">
        <button id="edit" class="btn brown-btn hide">Ändra</button>
    </div>
    <div id="content-wrapper" class="full-height"></div>
</main>

This is a template file that holds the markup for the "note block" look and also a text area where you write the note:

<div class="vertical-stripes"></div>
<div class="vertical-stripes"></div>
<ul id="note-block" class="reset">
    <li>
        <span>Idag</span>
        <span class="date"><?php include('datetime.php'); ?></span>
        <span><?php echo date('H:i'); ?></span>
    </li>
    <li></li>
    <li></li>
    <li></li>
    <li></li>
    <li></li>
    <li></li>
    <li></li>
    <li></li>
    <li></li>
    <li></li>
    <li></li>
    <li></li>
    <li></li>
    <li></li>
    <li></li>
    <li></li>
    <li></li>
</ul>
<form id="note-text" method="post" action="../includes/notepages/savenote.php">
    <textarea name="textarea" id="notes-area"></textarea>
</form>

When writing a note or editing it:

when you write a new or edit an existing note

A list of all the notes currently added:

all notes in a list with timestamp and cut-off topic

\$\endgroup\$
3
\$\begingroup\$

Your JavaScript code is nicely compartmentalized. It was a pleasure to read — though much more complex than necessary, as I shall explain below.

Repetitiveness in the event handlers

Your application has the following states:

  • View the list of notes
  • View a single note
    • Edit a single note

To answer your question directly, your event handlers are repetitive because you are writing all of the state transitions rather than the states themselves. Furthermore, the state is global. You can set the state as a CSS class on the root element, and let CSS take care of everything for you! The basic idea is as simple as

$('#notes-app').removeClass('mode-note').addClass('mode-list');

… and the CSS cascade takes care of the rest:

/* Top bar: mode state */
#notes-app.mode-list #all-notes,
#notes-app.mode-list #edit,
#notes-app.mode-list #done-btn {
    display: none;
}
#notes-app.mode-note #add-note,
#notes-app.mode-note.mode-view #done-btn,
#notes-app.mode-note.mode-edit #edit {
    display: none;
}

See the setMode() function below for details.

Structuring the DOM

The iOS Notes app has a peculiar property: the list view just shows excerpts of the first line of the content. That means that you can think of list mode and the read/edit mode as two views of the same content. When switching from list mode to read mode, simply hide all but the selected note, and let the mode-switching described above take care of the rest through CSS.

Editing interface

You have introduced an editing mode that is separate from viewing a note. It would be more user-friendly to make use of ContentEditable to let the user edit the content directly, instead of in a <textarea>. (I opted to leave vestiges of the edit mode in the demo below.)

Initialization

Setting window.onload = ctrl.functions.init; from within the $(document).ready(…) handler is weird. Arguably, the document has already finished loading at that point. So why not just call ctrl.functions.init(); directly?

Semantic markup

In your HTML, you had brown-btn as the name of a class. Don't do that, as brownness is a presentation-layer concept. For example, in iOS 7, the Notes application was completely re-skinned such that the buttons are not brown anymore. The DOM should not assert or imply that those UI elements are brown.

Demonstration

I've mocked up my solution below, minus the loading / editing / saving / deleting functionality. It works on WebKit and Firefox.

$(document).ready(function() {

    var ctrl = {

        bindElements: function() {
            $('#add-note').on('click', function() {
                ctrl.functions.selectNote(ctrl.functions.newNote());
                ctrl.functions.setMode('edit');
            });

            $('#all-notes, #done-btn').on('click', function() {
                ctrl.functions.viewList();
            });

            $('#edit').on('click', function() {
                ctrl.functions.setMode('edit');
            });

            $('#notes-list').on('click', 'li', function() {
                ctrl.functions.selectNote($(this));
            });

            $('#trashcan').on('click', function() {
                // TODO: Handle deletion
                ctrl.functions.viewList();
            });
        },

        functions: {
            init: function() {
                ctrl.bindElements();
                ctrl.functions.viewList();
            },
          
            viewList: function() {
                // TODO: Discard current note if empty; save it if modified
                $('#notes-list > li .note-content').prop('contenteditable', false);

                // Switch to list mode, and unhide all notes
                ctrl.functions.setMode('list');
                $('#notes-list > li').show();
            },
          
            selectNote: function($note) {
                // Hide all notes except the selected one, then switch modes
                $('#notes-list > li').hide()
                $note.show()
                     .children('.note-content')
                         .prop('contenteditable', true)
                         .focus();
                ctrl.functions.setMode('view');
            },

            newNote: function() {
                var now = new Date();
                var shortDate = now.getFullYear() + '-' +
                                ('0' + (now.getMonth()+1)).slice(-2) + '-' +
                                ('0' + now.getDate()).slice(-2) + ' ' +
                                ('0' + now.getHours()).slice(-2) + ':' +
                                ('0' + now.getMinutes()).slice(-2) + ':' +
                                ('0' + now.getSeconds()).slice(-2);
                var longDate = shortDate; // FIXME: use Date.toLocaleString() or something
                var $note = $(document.createElement('li'));
                $note.append('<div class="timestamp short">' + shortDate + '</div>' +
                             '<div class="timestamp long">' + longDate + '</div>' +
                             '<div class="timestamp friendly">Idag</div>' +
                             '<div class="note-content"></div>');
                $('#notes-list').append($note);
                return $note;
            },
          
            setMode: function(mode) {
                var $app = $('#notes-app');
                switch (mode) {
                  case 'list':
                    $app.removeClass('mode-note')
                        .addClass('mode-list');
                    break;
                  case 'view':
                    $app.removeClass('mode-edit')
                        .removeClass('mode-list')
                        .addClass('mode-view')
                        .addClass('mode-note');
                    break;
                  case 'edit':
                    $app.removeClass('mode-view')
                        .removeClass('mode-list')
                        .addClass('mode-edit')
                        .addClass('mode-note');
                    break;
                }
            },
        },
    };

    ctrl.functions.init();
});
body, #top-bar {
    margin: 0;
}
* {
    border: 0 solid rgb(141, 141, 137);
}
body {
    background-color: rgb(244, 242, 130);
    height: 100%;
    font-family: Helvetica, sans-serif;
}
#notes-list {
    list-style: none;
    font-family: "Marker Felt";
    margin: 0;
    padding: 0;
}
.timestamp {
    display: none;
    float: right;
    color: rgb(160, 77, 45);
    font-family: Helvetica, sans-serif;
}
.timestamp.friendly {
    float: none;
}

/* Top bar: mode state */
#notes-app.mode-list #all-notes,
#notes-app.mode-list #edit,
#notes-app.mode-list #done-btn {
    display: none;
}
#notes-app.mode-note #add-note,
#notes-app.mode-note.mode-view #done-btn,
#notes-app.mode-note.mode-edit #edit {
    display: none;
}

/* Top bar: styling */
#add-note, #done-btn, #edit {
    float: right;
}
#top-bar:after {
    display: block;
    content: ' ';
    clear: right;
}
#top-bar, input.btn, button.btn {
    background-color: rgb(103, 67, 57);
    background-image: none;
    color: white;
    font-weight: bold;
}
#top-bar .btn {
    margin: 2.5em 0.8em 0.8em 0.8em;
    border: 1px solid black;
    border-radius: 3px;
}
#top-bar {
    border-bottom-width: 1px;
}

/* Content: list mode */
#notes-app.mode-list li {
    padding: 0.5em 0 0.8em 0.8em;
    border-bottom-width: 1px;
    height: 1em;
    line-height: 150%;
    overflow: hidden;
}
#notes-app.mode-list .timestamp.short {
    display: block;
    margin-left: 3em;
    margin-right: 1em;
    font-weight: bold;
}
#notes-app.mode-list .note-content {
    white-space: nowrap;
    text-overflow: ellipsis;
    overflow: hidden;
}


/* Content: viewing/editing one note */
#notes-app.mode-note #notes-list > li {
    display: none;
}
#notes-app.mode-note .timestamp.long {
    display: block;
    font-size: 80%;
}
#notes-app.mode-note .timestamp.friendly {
    display: block;
    margin-left: 0;
    font-weight: bold;
}
#notes-app.mode-note #notes-list > li {
    margin: 0.2cm 0.5cm 0.5cm 2cm;
}
#notes-app.mode-note .note-content {
    font-size: 120%;
    margin-top: 0.8em;
    line-height: 130%;
    white-space: pre-wrap;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
<body id="notes-app" class="">
  <main class="full-height">
    <div id="top-bar">
        <input type="submit" form="note-text" value="Klar" id="done-btn" class="btn">
        <button id="edit" class="btn">Ändra</button>
        <button id="all-notes" class="btn">Alla anteckningar</button>
        <button id="add-note" class="btn">Ny</button>
    </div>
  </main>
  
  <ul id="notes-list" class="reset">
    <li>
      <div class="timestamp short">2015-01-06 16:22:35</div>
      <div class="timestamp long">6:e januari 16:22</div>
      <div class="timestamp friendly">Idag</div>
      <div class="note-content"
>var model = {
    ajaxURLs: {
        addNote: '../includes/notepages/addnote.php',
        editNote: '../includes/notepages/editnote.php',
        deleteNote: '../includes/notepages/deletenote.php',
        allNotes: '../includes/notepages/allnotes.php',
    },
    wrappers: {
        topBar: $('#top-bar'),
        content: $('#content-wrapper'),
        timestamp: undefined
    }
}</div>
    </li>

    <li>
      <div class="timestamp short">2015-01-05 16:22:33</div>
      <div class="timestamp long">5:e januari 16:22</div>
      <div class="timestamp friendly">Igår</div>
      <div class="note-content"
>model.wrappers.topBar.on('click', '#add-note', function() {

     var promise = ctrl.functions.restCall(model.ajaxURLs.addNote);
            promise.done(function(data) {

                $('#add-note').hide();

                ctrl.functions.showBtn('#all-notes');
                ctrl.functions.showBtn('#done-btn');

                ctrl.functions.appendData(data);
            });
    });</div>
    </li>
  </ul>
</body>

\$\endgroup\$
  • \$\begingroup\$ Superb feedback, a lot of useful pointers that I haven't thought about. Will make changes accordingly! Thank you very much man. Also accepted your answer instead as it was more useful than the previous accepted answer. \$\endgroup\$ – Chrillewoodz Jan 10 '15 at 10:18
  • \$\begingroup\$ My pleasure. This has been a fun project to work on. \$\endgroup\$ – 200_success Jan 10 '15 at 10:20
  • \$\begingroup\$ I know, that's why I decided to do it myself too :) \$\endgroup\$ – Chrillewoodz Jan 10 '15 at 10:22
  • \$\begingroup\$ Very good, I like it when I learn something new. If I include adding the word vestiges to my vocabulary, I learned two things. \$\endgroup\$ – Misunderstood Apr 9 '15 at 13:23
3
\$\begingroup\$

Are you sure that you need hideBtn? You forget to use it once and say

                $('#add-note').hide();

instead. But that's considerably shorter than

        ctrl.functions.hideBtn('#add-note');

Your hideBtn and showBtn functions don't seem to actually provide you anything that you are currently using. You could just as well leave them as hide() and show(). That leaves the problematic pattern, but at least it's not as verbose.

The usual fix for this pattern would be to make a single hideAll with something like

    hideAll: function() {
        $('#top-bar .btn').hide();
    },

Or just write:

        $('#top-bar .btn').hide();

Instead of using a separate function at all.

And then for show, you can use the arguments list:

    showBtns: function() {
        for ( var i = 0; i < arguments.length; i++ ) {
            $(arguments[i]).show();
        }
    },

Which you'd call like

        ctrl.functions.showBtns('#all-notes', '#edit');
        ctrl.functions.showBtns('#done-btn');

However, you don't always specify the button states. I haven't tried to track through the pattern, so I don't know if the buttons you don't specify stay hidden or stay shown. In fact, I'm not sure that it is known. It seems like there would likely be multiple paths to any particular point, so there would be multiple possible button arrangements. If this is correct (i.e. you shouldn't know which buttons are shown other than the ones that you explicitly hide or show), then I'm not sure that you have a DRY situation. You have a number of similar but different situations that don't respond mechanically to being combined.

If your current pattern is correct, then I believe that you are overthinking this. Your solution is likely to be more verbose and fragile than the problem. You already increase verbosity and indirection with your hideBtn and showBtn functions. Perhaps it would be better to step back and make things simpler rather than more complicated.

\$\endgroup\$
  • \$\begingroup\$ Very good point, didn't really see it that way. Seems redundant to create a function for a function and basically name it the same thing.. Will correct it thanks :) \$\endgroup\$ – Chrillewoodz Jan 7 '15 at 15:47

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.