5
\$\begingroup\$

Is there any way of improving this code? (Here's a JSFiddle with an example and comments)

Example: A hotel greeter can greet guests and accepts tips, but it's impossible to see the total of tips a greeter has received.

<!DOCTYPE html>
<html>
    <head>
    </head>
    <body>
        <pre>

            []___  
           /    /\     O <span id="alfred-says"><!-- the view --></span>
          /____/__\   /|\
          |[][]||||   / \
                     ALFRED

        </pre>
        <button onclick="javascript:alfred('greet')">Arrive at Hotel</button>
        <button onclick="javascript:alfred('acceptTip', 10)">Tip Alfred $10</button>
        <script>
        var HOTEL, alfred;
        HOTEL = {}; // Namespace
        (function (HOTEL) {
            // Reuseable controller
            var controllerPrototype = {
                greet: function () {
                    this.model.view.innerHTML = '"' + this.model.greeting + '"';
                },
                acceptTip: function (tip) {
                    this.model.tips += tip;
                    this.model.view.innerHTML = '"' + this.model.thankyou + '"';
                }
            };
            // Create a new greeter
            HOTEL.newGreeter = function (view) {
                var controller = Object.create(controllerPrototype);
                controller.facet = function () {
                    var args, method;
                    method = arguments[0];
                    args = 2 <= arguments.length ? [].slice.call(arguments, 1) : [];
                    if (method === "facet" || method === "model" || (controller[method] == null)) {
                        return undefined;
                    }
                    // Model, View and real Controller are never exposed
                    return controller[method].apply(controller, args);
                };
                controller.model = {
                    greeting: "Hello",
                    thankyou: "Thank you",
                    tips: 0,
                    view: view
                };
                return controller.facet;
            };
        }(HOTEL));
        // Alfred only exposes the controller facet. His tips are a secret...
        alfred = HOTEL.newGreeter(document.getElementById("alfred-says"));
        </script>
    </body>
</html>

Goals:

  • Keep it DRY
  • Keep it MVC
  • Make sure anyone with access to a "greeter" can never manipulate the view or model directly.

Limitations:

  • To avoid clutter, avoid type checking to avoid simple errors unless it is necessary to guarantee integrity.

Some specific thoughts:

  • Is there a better way of passing model and view into the controller?
  • Is there a better way of communicating with the controller?
  • Is there a good way of making a more general view?
  • Any other improvements?
\$\endgroup\$
11
  • \$\begingroup\$ by "principle of least concern" do you mean "principle of least knowledge"? or "principle of least effort"? \$\endgroup\$
    – nrw
    Commented Jun 17, 2014 at 15:13
  • \$\begingroup\$ I don't know where I got "Concerns" from, I meant to write "principle of least privilege", which I suppose is the same as or similar to "principle of least knowledge" \$\endgroup\$ Commented Jun 17, 2014 at 15:19
  • 2
    \$\begingroup\$ Principle of Least Knowledge, also known as the "Law of Demeter", has nothing to do with the "Principle of Least Privilege". Former is about not talking to strangers, latter is more related to security / authorisations. \$\endgroup\$ Commented Jun 17, 2014 at 15:34
  • \$\begingroup\$ My intention is that nothing with access to "Alfred" should be able to access or manipulate anything in a way that was not intended, so Principle of Least Knowledge/Law of Demeter is closer (although "has nothing to do with" is a bit of a stretch...). \$\endgroup\$ Commented Jun 17, 2014 at 15:54
  • 1
    \$\begingroup\$ @JosefOttosson in that case, isn't this mediator pattern a bit of a sledgehammer for a project which only does two very simple things (print a greeting and increment a counter)? If I reviewed this, my advice would be along those lines. \$\endgroup\$
    – Dagg
    Commented Jun 18, 2014 at 23:30

2 Answers 2

6
\$\begingroup\$

From the comments, I guess you would like a theoretical review ;)

  • This:

    <button onclick="javascript:alfred('greet')">Arrive at Hotel</button>
    <button onclick="javascript:alfred('acceptTip', 10)">Tip Alfred $10</button>
    

    in my mind should be wired by the controller, that is, the linking of UI elements to data and UI changes

  • This:

    greet: function () {
        this.model.view.innerHTML = '"' + this.model.greeting + '"';
    },
    

    has your controller access data straight in your view, updating the DOM. This is wrong in my mind. The view should have functions that the controller can call with either the model as a parameter or already passed in advance.

  • Object.create(controllerPrototype); <- Any reason you are eschewing new and prototype ?

  • Not that it truly matters but, method === "model" will not catch new String('model')

  • args = [].slice.call(arguments, 1); works as well as the ternary

All in all, I think this could use some more polishing.

\$\endgroup\$
2
\$\begingroup\$
  • Surely controllers need not understand what innerHTML is, that is a views job
  • Likewise controllers have no need to understand that a tip is an accumulated value, thats the models job
  • You dont really have a View (aka Mediator in PureMVC) in that code, you have a html element. A view should abstract away its implementation so the controller has no idea what is happening under the covers, and thereby does not get polluted by specific code.
  • A real view would be given a selector, know how to draw itself and its subcomponents, as well as wire up THEIR views (every component should have a view -- use the word Mediator here in your mind, 'view' makes you think wrong)
  • A Mediator should know nothing about the outside world, its job is to draw itself, expose methods for changing its display, expose methods for applying data, and emit events to a pub/sub system where a controller from a CONTROLLERS (plural) pool leaps into action to handle that one event.
  • Your Model has a view reference for pure convenience to you. This is wrong, there should be as I said, multiple mediators and if you do things right the one time you need to access them is in an event, by the appropriate controller -- so every event message has its source which is the Mediator.
  • The other time you need to access them is when the Model changes (also an event), so the controller corresponding to that event should get its mediator reference(s) from the registry of mediators (where mediators go to be born).
  • Your model is naked data, a model should have an accessor/CRUD layer to hide implementation. Model should NOT have validators (not that you do), that is the controllers job, but debatable.
  • All components of the MVC system should be registered and accessed via their registry, not through dot notation. They should be treated as if they are or may become transitory (you hold no refs to them, you get them, use them and let them go).

  • Take a look at how it is done in PureMVC, which is an easy to understand, small, extensible, language agnostic MVC.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.