1
\$\begingroup\$

I have created a module/factory/directive for AngularJS which allows to save user input to localStorage and restore it if needed.

Although it works quite good, I am sure that I could improve its code base and am looking for advice to do so.

It can be found on github: https://github.com/sprottenwels/saintAnthony.js

Code :

;var saintAnthony = angular.module('saintAnthony',[]);

saintAnthony.directive('savior',function(blackbox){
return{
    restrict: 'A',
    scope: true,
    link: function(scope,element,attrs){

        var elementData = {};

        var _getElementData = function(el){
            var data = {};
            var dotIndex = 0;

            el = el.isString ? el : el.toString();
            dotIndex = el.indexOf('.');

            data.container = attrs.savior !== "" ? attrs.savior : false;
            data.index = attrs.saviorindex !== undefined ? parseInt(attrs.saviorindex) : 0 ;
            data.mainGroup = dotIndex !== -1 ? el.substr(0,dotIndex) : el;
            data.key = el.substr(dotIndex +1,el.length);
            data.value = element[0].value;

            return data;
        };

        scope.saveSingleElement = function(){

            var obj = {};

            if (!blackbox.hasGroup(elementData.mainGroup)){
                blackbox.addGroup(elementData.mainGroup);
            }

            obj[elementData.key] = elementData.value;
            blackbox.updateGroup(elementData.mainGroup,obj);

        };

        scope.saveRepeatedElement = function(){
            var obj = {};

            if (!blackbox.hasGroup(elementData.container)){
                blackbox.addGroup(elementData.container,true);
            }

            obj[elementData.key] = elementData.value;
            blackbox.updateRepeatGroup(elementData.container,obj,elementData.index);
            console.dir(blackbox.groups);
        };


        element.bind('input',function(){

            elementData = _getElementData(attrs.ngModel);

            scope.$apply(function(){

                if(!elementData.container){scope.saveSingleElement();}
                else{scope.saveRepeatedElement();}

            });
        });

    }
}
});
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

my two cents :

  • add .idea to your .gitignore. If someone else uses the same IDE as you and works on this code, it could get very frustrating.

  • your code is not minification-proof, which can be annoying for someone who plans to use it. See angular's doc (section "a note on minification") for directions on how to fix this.

  • you dynamically extend String.prototype - this kind of monkey-patching can create conflicts with other libs, and lead to unexpected behaviors. If you want people to use your code, you should avoid this, especially for such simple code that you only use once.

  • you have two different entities defined in the same file, but that's ok as the whole lib is very lightweight. For more complex projects, split up your code, one entity per file.

  • your code lacks tests, which is a major no-go for many people.

  • instead of the quirky ng-change directive, you can directly set a change handler during the linking phase using $scope.$watch('something', function(){...}).

\$\endgroup\$
2
  • \$\begingroup\$ I've seldom seen a better criticsm. Thank you. Solely, i've got another question: Could you explain what makes the ng-change directive a bad one. \$\endgroup\$ Commented Oct 30, 2013 at 19:50
  • 1
    \$\begingroup\$ it's not bad per se, it's just that it hinders flexibility in this case : what if you want to add another handler than your one via ng-change ? Moreover, directives are here to encapsulate behaviour of your components ; here you ask people to know a bit too much about the internals of your directive. It's better to provide a default handler, and maybe let people override this via a data-attribute for instance \$\endgroup\$
    – m_x
    Commented Oct 30, 2013 at 20:09

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.