2
\$\begingroup\$

I have built the following application which searches Flickr API and returns photos. I am presenting the code:

index.html

<html ng-app="myapp">
<head>
  <meta charset="UTF-8" />
  <title>F1000 coding challenge</title>
  <script src="https://code.angularjs.org/1.5.3/angular.min.js"></script>
  <link rel="stylesheet" type="text/css" media="all" href="mine.css" />
</head>
<body>

 <section ng-controller="GalleryController as gallery" >
    <div id="legend">My Gallery</div>
    <div id="fr" search-directive></div>
    <div class="main-photo-area" main-photo-directive></div>
    <article> 
       <div photo-directive ng-repeat="photo in gallery.photos" ></div>
    </article>
    <div class="clear"></div>
    <nav footer-directive ng-repeat="member in gallery.divs"></nav>
  </section> 

<script src="js/service.js"></script>
<script src="js/controller.js"></script>
<script src="js/directive.js"></script>
</body>
</html>

controller.js

(function() {
  angular
    .module('myapp')  
    .controller('GalleryController', GalleryController);

  GalleryController.$inject = ['GalleryService'];

  function GalleryController(GalleryService){
     var self=this;
     self.current_page=1;      //page from JSON
     self.active=false;
     self.current_index=0;

     self.leftArrow=function(){
         if (self.photos.length!==0){
           if (self.current_index===0) {
               self.current_index=self.photos.length-1;
           } else {
               self.current_index--;
           }
           self.selected_photo=self.photos[self.current_index];
         }
     };

     self.rightArrow=function(){
         if (self.photos.length!==0){
           if (self.current_index===self.photos.length-1) {
               self.current_index=0;
           } else {
               self.current_index++;
           }
           self.selected_photo=self.photos[self.current_index];
         }
     };

     self.clickPhoto=function(index){
        self.current_index=index; 
        self.selected_photo=self.photos[index];
     };

     self.clickPage=function(value){
        if (value==="...") {
           return;
        } else if (value===">>") {
           self.current_page+=10;
           if (self.current_page>self.total_pages) {
               self.current_page=self.total_pages;
           }
        } else if (value==="<<") {
           self.current_page-=10;
           if (self.current_page<1) {
             self.current_page=1;
           }
        } else if (value==="Next") {
            self.current_page++;
        } else if (value==="Previous") {
            self.current_page--;
        } else {
            self.current_page=value; 
        }    

        if (self.total_pages>6) {
          if (self.current_page<self.total_pages-5) {
              self.divs[8].content=self.current_page;
              self.divs[7].content=self.current_page+1;
              self.divs[6].content=self.current_page+2;
              self.divs[5].content=self.current_page+3;
          } else {
              self.divs[8].content=self.total_pages-5;
              self.divs[7].content=self.total_pages-4;
              self.divs[6].content=self.total_pages-3;
              self.divs[5].content=self.total_pages-2;
          }
        }
        self.current_index=0;
        self.searchResults();
     };

     function footerDivs(pages){
        if (pages>6) {
           var divs=[
           {content:">>",classname:"class-numbers"},
           {content:"Next",classname:"class-numbers"},
           {content:pages,classname:"class-numbers"},
           {content:pages-1,classname:"class-numbers"},
           {content:"...",classname:"class-points"},
           {content:4,classname:"class-numbers"},
           {content:3,classname:"class-numbers"},
           {content:2,classname:"class-numbers"},
           {content:1,classname:"class-numbers"},
           {content:"Previous",classname:"class-numbers"},
           {content:"<<",classname:"class-numbers"} ];
        } else {
          var divs=[];  
          for (var i=pages;i>0;i--) {
              divs.push({content:i,classname:"class-numbers"});
          }
        } 
        return divs;
     }

     self.initialize=function(){
        self.searchResults().then(function() {
            self.current_index=0;   //as not to load temporarily the first image of the previous set
            self.current_page=1;
            var total=GalleryService.total_photos_number;
            self.total_pages=total%15===0 ? total/15 : (total-total%15)/15+1;
            self.divs=footerDivs(self.total_pages); 
        });
        self.active=true;
        self.current_page=1;
     };

     self.searchResults=function(){
         return GalleryService.getPhotos(self.search_tag,self.current_page).then(function() {
            self.photos = GalleryService.photos;
            self.selected_photo=self.photos[0];
         });
     };

     self.searchResults().then(function(){
         var total=GalleryService.total_photos_number;
         self.total_pages=total%15===0 ? total/15 : (total-total%15)/15+1;
         self.divs=footerDivs(self.total_pages);
     });
  } 
})();

service.js

(function() {
  angular
    .module('myapp',[])  
    .service('GalleryService', GalleryService);

  GalleryService.$inject = ['$http'];

  function GalleryService($http){
    var url = "https://api.flickr.com/services/rest/";
    var self=this;

//https://api.flickr.com/services/rest/?method=flickr.photos.search&api_key=c4e2f731926eefa6fe1d3e9c2c9f9449&tags=chocolate&per_page=15&page=2&format=json&jsoncallback=JSON_CALLBACK
    this.getPhotos=function(choice,page) {
      var params_value={    
        api_key:'c4e2f731926eefa6fe1d3e9c2c9f9449',
        per_page:15,
        page:page,
        format:'json',
        jsoncallback:'JSON_CALLBACK'
      };
      if (choice==='' || choice===undefined) {
        params_value.method= 'flickr.photos.getRecent'; 
      } else {
         params_value.tags=choice;
         params_value.method='flickr.photos.search'; 
      }
      return $http.jsonp(url, {
        params: params_value
      }).then(handleResponse,handleError);
    };

    function handleResponse(response) { 
      self.photos = retrievePhotoURLs(response.data);
      self.total_photos_number=response.data.photos.total;
    }  

    function handleError() {
      console.log('Error retrieving JSON data');    
    }

    function retrievePhotoURLs(data) {
        return data.photos.photo.map(constructLink);
    }

    function constructLink(element){
        //http://farm{farm-id}.staticflickr.com/{server-id}/{id}_{secret}.jpg
        return "http://farm"+element.farm+".staticflickr.com/"+element.server+"/"+element.id+"_"+element.secret+".jpg";
    }
  }
})();

directive.js

(function() {
  angular
    .module('myapp')  
    .directive('searchDirective', searchDirective)
    .directive('mainPhotoDirective', mainPhotoDirective)
    .directive('photoDirective', photoDirective)
    .directive('footerDirective', footerDirective);

  function searchDirective(){
     return {
         templateUrl:'frame1.html'   
     }
  } 

  function mainPhotoDirective(){
     return {
         templateUrl:'frame2.html'   
     }
  } 

  function photoDirective(){
     return {
         templateUrl:'frame3.html'   
     }
  } 

  function footerDirective($compile){
      return {
         templateUrl:'frame4.html'   
     }
  } 
})();

frame1.html

<input  type="text" ng-model="gallery.search_tag" ng-change="gallery.active=false" ng-keypress="$event.which === 13 && gallery.initialize()" 
placeholder="Search"/>
<div id="icon1" ng-click="gallery.initialize()" ng-class="{'visible':gallery.active}">
 <img src="img/magnifier.png"/>
</div>
<div id="icon2" ng-click="gallery.search_tag='';gallery.active=false" ng-class="{'visible':!gallery.active}">
 <img src="img/clear-button.png"/>
</div>

frame2.html

<div id="left-arrow" ng-click="gallery.leftArrow()">
  <img src="img/left-arrow.png" /> 
</div>
<div id="right-arrow" ng-click="gallery.rightArrow()">
  <img src="img/right-arrow.png" /> 
</div>  
<div class="main-photo-frame" ng-class="{'visible':gallery.photos.length===0}">  
  <div><img ng-src="{{gallery.photos[gallery.current_index]}}" alt="broken link"> </div>
  <div class="title">IMG{{gallery.current_index+1}}</div>
</div>

frame3.html

<div class="photo-frame" ng-click="gallery.clickPhoto($index)" ng-class="{'selected':gallery.selected_photo===photo}"> 
  <img ng-src="{{photo}}" alt="broken link">
  <div>IMG{{$index+1}}</div>
</div>

frame4.html

<div class="{{member.classname}} "ng-click="gallery.clickPage(member.content)" 
ng-class="{'clicked':gallery.current_page===member.content && member.content!=='...','visible': member.content==='Previous' && gallery.current_page===1,
'displayed': member.content==='Next' && gallery.current_page===gallery.total_pages}"> 
   {{member.content}}
</div>

mine.css

@charset "utf-8";

html {
  font-family: sans-serif;
}
body{
    margin: 20px;
    background-color: #286090;
    padding-top:50px;
}
section{
    overflow:hidden;
    font-weight: bold;
    font-style:italic;
    font-size:16px;
    padding:50px;
    background-color: #FFF;
    border: 1px solid #ccc;;
    border-radius: 4px;
    min-height:500px;
    max-width:900px;
    margin:0 auto;
}
#fr{
    float:right;
    position:relative;
    background-color: #286090;
    padding:8px;
}
#icon1{
    position:absolute;
    cursor:pointer;
    right:15px;
    top:15px;
}
#icon2{
    position:absolute;
    cursor:pointer;
    right:15px;
    top:15px;
}
.visible,.displayed{
    display:none;
}
input[type="text"] {    
    border:0;
    border-radius:4px;
    color: #444;
    font-size: 1em;
    width: 330px;
    height:30px;
    background-color: #FFF;
    padding-left: 5px;
    padding-right: 30px;
    padding-bottom:0;
    padding-top:0;
}
#legend{
    float:left;
    font-size:20px; 
}
.main-photo-area{
    clear:both;
    position:relative;
    margin-top:70px;
    padding-top:35px;
    padding-bottom:15px;
    background-color: #286090;
    height:400px;
}
#left-arrow{
    position:absolute;
    top:200px;  
    left:15px;
}
#left-arrow:hover,#right-arrow:hover{
    box-shadow:0 0 5px 3px #FFF;
    cursor:pointer;     
}
#right-arrow{
    position:absolute;
    top:200px;  
    right:15px;
}
.main-photo-frame{
    width:70%;
    height:400px;
    font-size:16px;
    color:#FFF;
    margin:0 auto;
    text-align:center;
}
.main-photo-frame img{
    height:96%;
    display: block;
    width:100%; 
}
.title{
    margin-top:5px; 
}
article{
    margin-top:30px;
    margin-left:-0.5%;
    margin-right:-0.5%;
}
article img{
    height:180px;
    display: block;
    width:100%; 
}
.photo-frame{
    -webkit-box-sizing: border-box;
    -moz-box-sizing: border-box;
    box-sizing: border-box;
    -ms-box-sizing: border-box;
    width:19%;
    height:205px;
    font-size:12px;
    float:left;
    margin-left:0.5%;
    margin-right:0.5%;
    margin-bottom:20px;
    padding-top:4px;
    text-align:center;
}
.photo-frame:hover,.selected{
    border:4px solid #000;  
    padding-top:0;
}
.clear{
    clear:both;
}
.class-numbers,.class-points{
    float:right;
    font-size:15px;
    margin-left:10px;
    margin-right:10px;
    padding:5px;
    font-family:Cambria, "Hoefler Text", "Liberation Serif", Times, "Times New Roman", serif;
    font-style:normal;
}
.class-numbers:hover,.clicked{
    cursor:pointer;
    background-color:#777;
}

I did really my best to build the above code and I was sure it is well structured, readable and maintainable. However, the criticism I received is that the code is not up to a good standard. I am about to go crazy. Do you think this criticism is fair? If yes, what are the points that denote the code is not good?

\$\endgroup\$
2
  • \$\begingroup\$ Were there any specific criticisms? \$\endgroup\$
    – Peter Carter
    Commented May 19, 2016 at 8:54
  • 1
    \$\begingroup\$ No, just the above \$\endgroup\$
    – Unknown developer
    Commented May 19, 2016 at 15:06

2 Answers 2

Reset to default
2
\$\begingroup\$

Personally, I think overall the code is of sufficient quality. I prefer the angular 1.5 component architecture, but that's more my opinion. I did notice one line that was slightly questionable or not of suggested style.

if (choice==='' || choice===undefined) {...}

Specifically, the test for undefined. According to this Stack Overflow answer the suggested way to test for undefined would be something like this:

if (choice==='' || typeof choice == 'undefined') {...}
\$\endgroup\$
2
+50
\$\begingroup\$

Overall it looks good and readable but we should try to avoid nested if-else statements for simple condition by using ternary operator. say for your leftArrow function you can use it as below : 

if (self.photos.length!==0){
    self.current_index = (self.current_index === 0) ? self.photos.length - 1 : self.current_index - 1;
    self.selected_photo = self.photos[self.current_index];
}

Also we can try to avoid if-else statements by replacing it with switch case where you have to compare only one variable value. You can replace your "clickPage" function to use switch case because every time you are checking only one variable value in it.

You can also check your jsfile with https://www.npmjs.com/package/eslint which will give you some more optimization suggestion for your js code which will be helpful when we minify js.

\$\endgroup\$

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.