7
\$\begingroup\$

I have to two datepickers. I want achieve the following functionality: the return date cannot be selected before the selected departure date and the departure date cannot be after the selected return date. Mainly I am interested in correctness of my angular architecture.

I have moved datepicker into my custom directive which I plan to reuse many times for different screen sizes.

Both datepickers reuse the same DatepickerContoller and I have one parent controller FormDatepickerController which shares information from both outbound and return datepickers.

Could you please review my code and let me know if my design is good enough?

index.html

<div ng-controller="FormDatepickerController as fd">
    <div ng-controller="DatePickerController as dp">
        <div class="col-md-2">
            <date-picker date="fd.departDate" min-date="fd.departDate"></date-picker>
        </div>

        <div class="col-md-2">
            <date-picker date="fd.returnDate" min-date="fd.departDate"></date-picker>
        </div>
    </div>
</div>

datepicker.html template

<div class="input-group">
    <input type="text" class="form-control"
           datepicker-popup
           min-date="minDate"
           ng-model="date"
           is-open="isOpened"
           ng-required="true"
           close-text="Close" />
            <span ng-click="open($event)" class="btn btn-default input-group-addon">
                <span class="glyphicon glyphicon-calendar"></span>
            </span>

</div>

app.js

var app = angular.module("fridayFly", ['ui.bootstrap']);

datepicker-controller.js

var app = angular.module("fridayFly");

(function(){

    function Controller($scope) {
        $scope.open = function($event) {
            $event.preventDefault();
            $event.stopPropagation();

            $scope.isOpened = true;
        };


        $scope.today = function() {
            $scope.dt = new Date();
        };
        $scope.today();

        $scope.clear = function () {
            $scope.dt = null;
        };

        $scope.toggleMin = function() {
            $scope.minDate = $scope.minDate ? null : new Date();
        };
        $scope.toggleMin();

        $scope.dateOptions = {
            formatYear: 'yyyy',
            startingDay: 1
        };

        $scope.formats = ['dd-MMMM-yyyy', 'yyyy/MM/dd', 'dd.MM.yyyy', 'shortDate'];
        $scope.format = $scope.formats[0];

    }

    app.controller('DatePickerController', Controller);


})();

(function(){

    function directive(){
        return {
            scope :{
                date:'=date',
                minDate:'=minDate',
                isOpened:'=isOpened'
            },
            restrict: 'E',
            templateUrl: 'templates/datepicker/datepicker.html',
            controller: 'DatePickerController'
        };
    };

    app.directive('datePicker', directive);

})();

form-datepicker-contoller.js

var app = angular.module("fridayFly");


app.controller('FormDatepickerController', ['$scope', function($scope) {

    $scope.toggleDepartDate = function() {
        $scope.departDate = $scope.departDate ? null : new Date();
    };
    $scope.toggleDepartDate();

    $scope.toggleReturnDate = function() {
        $scope.returnDate = $scope.returnDate ? null : new Date();
    };
    $scope.toggleReturnDate();

}]);
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! I hope you get some great answers! \$\endgroup\$ – Phrancis Aug 5 '15 at 22:03
1
\$\begingroup\$

Excess controllers?

This code appears to be similar to several examples of UI Bootstrap datepickers - e.g. this plunker. Initially the layout looked fine but then I realize that there are three total instances of DatePickerController - one for each <date-picker> plus the wrapping <div ng-controller="DatePickerController as dp">. My presumption is that you should be able to implement this without that wrapping DatePickerController but maybe there is something I am missing.

Descriptive function/class names

I noticed that the functions for the controller and the directive are named Controller and directive, respectively. Most widely-accepted style guides call for class names in UpperCamelCase while method/function names should be in lowerCamelCase (e.g. the Google Style Guide). As your application grows it would be advisable to make the function names more descriptive - e.g. datePickerController and datePickerDirective.

Functions only called once

Do the methods toggleDepartDate() and toggleReturnDate() in FormDatepickerController get called anywhere other than the controller initializer (i.e. immediately after they are declared)? If not, then the functionality within them could be moved outside the functions.

IIFEs could be combined

There are two IIFEs in datepicker-controller.js. Those could be combined into a single IIFE because there isn't any reason to have them separated (at least code-wise).

\$\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.