1
\$\begingroup\$

In my Angular app, users click on different icons. When a user clicks on respective icon, I can show the icons gallery, as well shuffle the view.

Apart from this shuffling functionality, I need to hide whatever other gallery is already open.

For that, I've written the following function, but, I think it could use some refactoring.

$scope.galleryMenu = function ( GalleryType ) {

            var hideGallery = (function (type) {

                return {

                    galleryVideoShow : function () {

                        //closing others
                        $scope.contractorShow = false;
                        $scope.galleryPhotoShow = false;
                    },

                    contractorShow : function () {
                        //closing others
                        $scope.galleryPhotoShow = false;
                        $scope.galleryVideoShow = false;

                    },

                    galleryPhotoShow : function () {

                       //closing others
                        $scope.contractorShow = false;
                        $scope.galleryVideoShow = false;

                    }

                }

            })();

            $scope.$watch('viewProjectInfo', function ( newValue, oldValue ) {


                if(!newValue) return;

                $scope.photos = $scope.viewProjectInfo.ImagePaths;

                $scope[GalleryType] = !$scope[GalleryType];
                $scope.shownGallery =  GalleryType;

                hideGallery[GalleryType](); //manually close other open gallery

                $('html, body').animate({scrollTop:$(document).height()}, 'slow');



            });



        };
\$\endgroup\$
1
\$\begingroup\$

A few things that could be improved:


You have quite a lot of empty white lines, and extraneous space:

return {

    galleryVideoShow : function () {

        //closing others
        $scope.contractorShow = false;
        $scope.galleryPhotoShow = false;
    },

    contractorShow : function () {

    // ...

This block, for example, without extraneous whitespace, would look like:

return {
    galleryVideoShow: function(){
        $scope.contractorShow,
        $scope.galleryPhotoShow = false;
    },
    contractorShow: function(){

    //...

        var hideGallery = (function (type) {

            return {

                galleryVideoShow : function () {

                    //closing others
                    $scope.contractorShow = false;
                    $scope.galleryPhotoShow = false;
                },

                contractorShow : function () {
                    //closing others
                    $scope.galleryPhotoShow = false;
                    $scope.galleryVideoShow = false;

                },

                galleryPhotoShow : function () {

                   //closing others
                    $scope.contractorShow = false;
                    $scope.galleryVideoShow = false;

                }

            }

        })();

There's no reason to assign hideGallery like that.

It'd be better as a plain object.

var hideGallery = {
        galleryVideoShow: function(){
            //closing others
            $scope.contractorShow,
            $scope.galleryPhotoShow = false;
        },
        contractorShow: function(){
            $scope.galleryPhotoShow,
            $scope.galleryVideoShow = false;
        },
        galleryPhotoShow: function(){
            $scope.contractorShow,
            $scope.galleryVideoShow = false;
        }
}

Which would be better called something like GALLERY_EFFECTS, and kept as a constant.

Also note that for each function in hideGallery, the comment //closing others was present, it's only part of what the functions do, and would be better off at the root of hideGallery in a more expanded version.


       $scope.$watch('viewProjectInfo', function ( newValue, oldValue ) {


            if(!newValue) return;

            $scope.photos = $scope.viewProjectInfo.ImagePaths;

            $scope[GalleryType] = !$scope[GalleryType];
            $scope.shownGallery =  GalleryType;

            hideGallery[GalleryType](); //manually close other open gallery

            $('html, body').animate({scrollTop:$(document).height()}, 'slow');



        });



    };
  • Why are there so many empty lines at the bottom? (six, I counted) They just look poorly formatted, or like missing code.
  • You should wrap your conditionals in brackets, because untold circumstances can happen.
  • You should spread the .animate call over a multiple lines, as you're building an object inside the function call.

   $scope.$watch('viewProjectInfo', function(newValue, oldValue){
       if (!newValue){ return };
       $scope.photos = $scope.viewProjectInfo.ImagePaths;

       $scope[GalleryType] = !$scope[GalleryType];
       $scope.shownGallery = GalleryType;

       hideGallery[GalleryType]();

       $('html, body').animate({
           scrollTop: $(document).height()
       }, 'slow');
   });
\$\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.