0
\$\begingroup\$

I'm using the MEAN stack to make a small chat app, which will soon support braided messaging. This is my first Angular app so I am not sure how I'm doing on the front end portion in terms of keeping it neat and organized appropriately (might be too monolithic). I have not used any directives, and my HTML feels like it could be split up which seems like something directives would help do.

Additionally, I only have one controller and don't fully understand when you would use multiple controllers, so I wonder if that's too monolithic as well. Those are my specific concerns but also would love other feedback because I imagine there is tons to improve!

public/views/index.html

<nav class="navbar navbar-default">
    <div class="container-fluid">
        <div class="navbar-header navbar-right">
            <a class="navbar-brand">{{ selected_user.username }}</a>
        </div>
    </div>
</nav>

<div class="container-fluid">
<div id="margin-fixer-grid-wrapper">

    <div class="row">

        <div id="convo-list-pane" class="col-sm-2">

            <div class="row">
                <div class="col-sm-12">
                    <h3>Convos</h3>
                </div>
            </div>

            <div class="row">
                <div class="col-sm-12">
                    <div class="convo-in-list" ng-class="{selected: convo._id==selected_convo._id}" ng-click="selectConvo(convo)" ng-repeat="convo in convos">
                        <b>{{ convo | partner:selected_user | username:username_map }}</b>
                        <button ng-click="deleteConvo(convo._id, selected_user._id); $event.stopPropagation();">X</button>
                    </div>
                </div>
            </div>

            <div id="convo-form" class="row">
                <form>
                    <div class="col-sm-9">
                        <div class="form-group">
                            <select ng-model="forms.newConvoFormData.user_id_1" ng-options="convo_partner._id as convo_partner.username for convo_partner in potential_partners"></select>
                        </div>
                    </div>

                    <div class="col-sm-3">
                        <button type="submit" class="btn btn-primary" ng-click="createConvo()">+</button>
                    </div>
                </form>
            </div>

        </div>

        <div id="message-list-pane" class="col-sm-8">

            <div class="row">
                <div class="col-sm-12">
                    <h3>Messages</h3>
                </div>
            </div>

            <div class="row">
                <div class="col-sm-12">
                    <h4>{{ selected_convo | partner:selected_user | username:username_map }}</h4>
                </div>
            </div>

            <div class="row">
                <div class="col-sm-12">
                    <div ng-repeat="message in messages">
                        <button ng-click="deleteMessage(message._id, selected_convo._id)">X</button><p> {{ message.sender_id }}: {{ message.text }}</p>
                    </div>
                </div>
            </div>

            <div class="row">
                <div class="col-sm-12">
                    <form>
                        <div class="col-sm-8">
                            <div class="form-group">
                                <input type="text" class="form-control input-lg" ng-model="forms.newMessageFormData.text">
                            </div>
                        </div>
                        <div class="col-sm-4">
                            <button type="submit" class="btn btn-primary btn-lg" ng-click="createMessage()">Send</button>
                        </div>
                    </form>
                </div>
            </div>

        </div>

        <div id="user-list-pane" class="col-sm-2">

            <div class="row">
                <div class="col-sm-12">
                    <h3>Users</h3>
                </div>
            </div>

            <div class="row">
                <div class="col-sm-12">
                    <div class="user-in-list" ng-class="{selected: user._id==selected_user._id}" ng-click="selectUser(user)" ng-repeat="user in users">
                        <p>User: <b>{{ user.username }}</b></p>
                        <button ng-click="deleteUser(user._id); $event.stopPropagation();">X</button>
                    </div>
                </div>
            </div>

            <div id="user-form" class="row">
                <form>
                    <div class="col-sm-9">
                        <div class="form-group">
                            <input type="text" class="form-control" ng-model="forms.newUserFormData.username">
                        </div>
                    </div>
                    <div class="col-sm-3">
                        <button type="submit" class="btn btn-primary" ng-click="createUser()">+</button>
                    </div>
                </form>
            </div>

        </div>

    </div>

</div>
</div>

public/controllers/js/main.js

angular.module('braidController', [])

.controller('mainController', ['$scope', '$http', 'Convos', 'Messages', 'Users', function($scope, $http, Convos, Messages, Users) {


    // initialize variables

    $scope.messages = [];
    $scope.convos = [];
    $scope.users = [];
    $scope.selected_convo = undefined;
    $scope.selected_user = undefined;
    $scope.potential_partners = [];
    $scope.username_map = {};
    $scope.forms = {
        newMessageFormData: {},
        newConvoFormData: {},
        newUserFormData: {}
    }

    Users.get()
        .success(function (data) {
            $scope.users = data;
            $scope.selected_user = $scope.users[0];
        });


    // define functions used in the template

    $scope.createMessage = function() {
        $scope.newMessageFormData.convo_id = $scope.selected_convo._id;
        $scope.newMessageFormData.sender_id = $scope.selected_user._id;
        if ($scope.selected_convo.user_id_0 = $scope.selected_user._id) {
            $scope.newMessageFormData.receiver_id = $scope.selected_convo.user_id_1;
        } else {
            $scope.newMessageFormData.receiver_id = $scope.selected_convo.user_id_0;
        };
        $scope.newMessageFormData.time_sent = new Date();

        if ($scope.newMessageFormData.text) {

            Messages.create($scope.newMessageFormData)
                .success(function(data) {
                    $scope.newMessageFormData = {};
                    $scope.messages = data;
                });

        };
    };

    $scope.deleteMessage = function(message_id, convo_id) {

        Messages.delete(message_id, convo_id)
            .success(function(data) {
                $scope.messages = data;
            });

    };

    $scope.selectConvo = function(convo) {
        $scope.selected_convo = convo;
    };

    $scope.createConvo = function() {
        $scope.newConvoFormData.user_id_0 = $scope.selected_user._id;

        if ($scope.newConvoFormData.user_id_1) {

            Convos.create($scope.newConvoFormData)
                .success(function(data) {
                    $scope.newConvoFormData = {};
                    $scope.convos = data;

                    if (!$scope.selected_convo) {
                        $scope.selected_convo = $scope.convos[0];
                    };
                });

        };
    };

    $scope.deleteConvo = function(convo_id, user_id) {

        Convos.delete(convo_id, user_id)
            .success(function(data) {
                $scope.convos = data;

                if (convo_id == $scope.selected_convo._id) {
                    $scope.selected_convo = $scope.convos[0];
                };
            });

    };

    $scope.selectUser = function(user) {
        $scope.selected_user = user;
    }

    $scope.createUser = function() {
        if ($scope.newUserFormData.username) {

            Users.create($scope.newUserFormData)
                .success(function(data) {
                    $scope.newUserFormData = {};
                    $scope.users = data;

                    if (!$scope.selected_user) {
                        $scope.selected_user = $scope.users[0];
                    };
                });

        };
    };

    $scope.deleteUser = function(user_id) {

        Users.delete(user_id)
            .success(function(data) {
                $scope.users = data;

                if (user_id == $scope.selected_user._id) {
                    $scope.selected_user = $scope.users[0];
                };
            });

    };


    // register listeners

    var refreshMessages = function() {
        if ($scope.selected_convo) {

            Messages.get($scope.selected_convo._id)
                .success(function(data) {
                    $scope.messages = data;
                });

        } else {
            $scope.messages = [];
        };
    };

    var refreshConvos = function() {
        if ($scope.selected_user) {

            Convos.get($scope.selected_user._id)
                .success(function(data) {
                    $scope.convos = data;
                    $scope.selected_convo = $scope.convos[0];
                });

        } else {
            $scope.convos = [];
            $scope.selected_convo = undefined;
        };
    };

    var refreshPotentialPartners = function() {
        var already_convod = [];
        _.each($scope.convos, function(convo) {
            already_convod.push(convo.user_id_0, convo.user_id_1);
        });
        $scope.potential_partners = $scope.users.filter(function(user) {
            return (($.inArray(user._id, already_convod) == -1) && (user._id != $scope.selected_user._id));
        });
    };

    var refreshUserIdToUsernameMap = function() {
        var temp_user_map = {};
        _.each($scope.users, function(user) {
            temp_user_map[user._id] = user.username;
        });
        $scope.username_map = temp_user_map;
    };

    $scope.$watch('selected_convo', refreshMessages);
    $scope.$watch('selected_user', refreshConvos);
    $scope.$watchGroup(['convos', 'users', 'selected_convo'], refreshPotentialPartners);
    $scope.$watch('users', refreshUserIdToUsernameMap);

}]);

Those are the relevant files for my specific concerns, but here is a link to my GitHub repo, just in case there are details I left out which would be helpful.

\$\endgroup\$
1
\$\begingroup\$

I could come up with a few generic suggestions:

First of all, you have to try to have your controllers as skinny as possible. In your case there is muchy logic going on that can be moved to directives mostly.

Since is not a huge page, you may make it with just the controller you have.

The beginning code of the navbar can go on a navbar directive with user or username as parameter, so you can reuse on other pages

The code for the messages can definitely go in a directive, so you'll pass from what you had:

<div id="message-list-pane" class="col-sm-8">

        <div class="row">
            <div class="col-sm-12">
                <h3>Messages</h3>
            </div>
        </div>

        <div class="row">
            <div class="col-sm-12">
                <h4>{{ selected_convo | partner:selected_user | username:username_map }}</h4>
            </div>
        </div>

        <div class="row">
            <div class="col-sm-12">
                <div ng-repeat="message in messages">
                    <button ng-click="deleteMessage(message._id, selected_convo._id)">X</button><p> {{ message.sender_id }}: {{ message.text }}</p>
                </div>
            </div>
        </div>

        <div class="row">
            <div class="col-sm-12">
                <form>
                    <div class="col-sm-8">
                        <div class="form-group">
                            <input type="text" class="form-control input-lg" ng-model="forms.newMessageFormData.text">
                        </div>
                    </div>
                    <div class="col-sm-4">
                        <button type="submit" class="btn btn-primary btn-lg" ng-click="createMessage()">Send</button>
                    </div>
                </form>
            </div>
        </div>

    </div>

To something like:

<div id="message-list-pane" class="col-sm-8">
   <message-pane convo="selected_convo" user="selected_user"></message-pane>
</div>

Then you can later reuse the message pane, plus all the interaction with messages will be handled in the directive, outside the controller (good).

Once you have extracted that logic, you may consider handy to do the same for the convo-list-pane and user-list-pane as well, extracting directives on the whole HTML inside, or parts of it as you may see fit.

Each $watch can go inside each directive as well.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This is fantastic. Thank you so much! Exactly what I was hoping for. First time using Code Review Exchange is a success. \$\endgroup\$ – tscizzle Mar 4 '16 at 15:27

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.