Shuffling and hiding views specified by parameters

Question

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');
 });
 };

Answer 1

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');
 });