3
\$\begingroup\$

I am working on an angular application. I have written a controller for a page which gets 10 objects from an api. On clicking the next button, it gets the next 10 objects and so on. I have also added a search box in the same controller which also gets 10 results of the searched term and on clicking the next button it gets the next 10 results.

HTML -

<!-- Show results -->
<div class="panel panel-default jobs" ng-repeat="job in jobs.jobsList">
    <div class="panel-heading text-center"><a href="{{job.career_url}}" target='_blank' class="hvr-sink"><h3 class="well">{{job.job_name}}</h3></a></div>
    <div class="panel-body text-center flexcontainer">
        <div>Company: {{job.company_name}}</div>
    </div>
</div>

<!-- Search buton and search box -->
<span class="searchButton"><i class="fa fa-search fa-2x"></i></span><input ng-change="jobs.search()" ng-model="jobs.searchTerm" ng-keydown="jobs.deleteTerm($event)" type="text" id="search-box" style="width: 0px; visibility:hidden;"/>

<!-- Take to top of page button -->
<a href="#" id="back-to-top" title="Back to top"><i class="fa fa-arrow-up fa-2x"></i></a>

<!-- Get next ten results -->
<div ng-click="jobs.getNext()" class="nextButton">See More</div>

<script type="text/javascript"> 
    var toggleVar = true;
    $('.searchButton').on('click', function() {
        if(toggleVar) {
            $('.searchButton').animate({right: '210px'}, '1200');
            $('#search-box').css("visibility", "visible");
            setTimeout(function() {
                $('.searchButton').css("color", "#444444");
            }, 100);
            $('#search-box').animate({ width: 185 }, '1200').focus();
            toggleVar = false;
        }
        else {
            $('#search-box').animate({ width: 0 }, '1200');
            $('.searchButton').animate({right: '25px'}, '1200');
            setTimeout(function() {
                $('.searchButton').css("color", "#eeeeee");
            }, 200);
            toggleVar = true;
        }
    });

    $('#search-box').focusout(function() {
        if(!toggleVar) {
            $('#search-box').animate({ width: 0 }, '1200');
            $('.searchButton').animate({right: '25px'}, '1200');
            setTimeout(function() {
                $('.searchButton').css("color", "#eeeeee");
            }, 200);
            toggleVar = true;
        }
    });
</script>

Controller -

angular.module('jobSeekerApp')
  .controller('JobsallCtrl', ['getAllJobs', 'searchJobs', function (jobsService, jobsSearchService) {
    var ctrl = this;
    var count;
    ctrl.pageNumber = 1; 
    ctrl.searchPageNumber = 1;
    ctrl.isSearching = false;
    ctrl.searchTerm = "";

    // Initial page load
    jobsService.getJobs(ctrl.pageNumber)
      .then(function(response) {
        ctrl.jobsList = response.data.results;
        count = response.data.count;
        checkCount();
      }, function(error) {
        console.log(error);
      });

    // User clicks next button
    ctrl.getNext = function() {
      // If search is not being used
      if(ctrl.searchTerm === "" && ctrl.isSearching === false) {
        ctrl.pageNumber = ctrl.pageNumber + 1;
        jobsService.getJobs(ctrl.pageNumber)
          .then(function(response) {
            ctrl.jobsList = ctrl.jobsList.concat(response.data.results);
            checkCount(); 
          }, function(error) {
            console.log(error);
          });
      }
      // If search is being used
      else {
        ctrl.searchPageNumber = ctrl.searchPageNumber + 1;
        jobsSearchService.searchJob(ctrl.searchPageNumber, ctrl.searchTerm)
          .then(function(response) {
            ctrl.jobsList = ctrl.jobsList.concat(response.data.results);
            checkCount();
          }, function(error) {
            console.log(error);
          });
      } 
    };

    // User backspaces to delete search term
    ctrl.deleteTerm = function (event) {
      if (event.keyCode === 8) {
        ctrl.searchTermLen = ctrl.searchTermLen - 1;
      }
      // If search box is empty
      if(ctrl.searchTermLen === 0) {
        ctrl.isSearching = false;
      }
    };

    // User clicks search button
    ctrl.search = function() {
      ctrl.searchTermLen = ctrl.searchTerm.length;
      // If search box is empty, show normal results
      if(ctrl.searchTerm === "" && ctrl.isSearching === false) {
        ctrl.pageNumber = 1;
        jobsService.getJobs(ctrl.pageNumber)
          .then(function(response) {
            ctrl.jobsList = response.data.results;
            count = response.data.count;
            checkCount();
          }, function(error) {
            console.log(error);
          });
      }
      // If search box is not empty, search the input
      else {
        ctrl.isSearching = true;
        ctrl.searchPageNumber = 1;
        jobsSearchService.searchJob(ctrl.searchPageNumber, ctrl.searchTerm)
          .then(function(response) {
            ctrl.jobsList = response.data.results;
            count = response.data.count;
            checkCount();
          }, function(error) {
            console.log(error);
          });
      }
    };

    // Function to hide and show next button
    function checkCount() {
      console.log(count);
      if(count < 10) {
        $(".nextButton").hide();
      }
      else {
        $(".nextButton").show();
      }
      count = count - 10;
    }
  }]);

I have added separate services for normal results and search results. Since the search box hides when out of focus and angular starts getting normal results after instead of search results, I added isSearching variable so it knows when the user is searching and when not.

Now, the application is working fine. But I feel there is a lot of repetition in the code and overall the quality is not good. I dont have much experience with JavaScript or Angular, so could anyone help with how i can improve the code. I think I could also use custom directives, but I am not sure where and for what functionality.

\$\endgroup\$
  • \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ – BCdotWEB Aug 10 '16 at 8:12
  • \$\begingroup\$ I wouldn't recommend putting swearing in your console.log. Try and clean up your code of obscenities before showing it to other people at least; it doesn't reflect well on you otherwise. \$\endgroup\$ – Dan Pantry Aug 10 '16 at 8:47
  • 1
    \$\begingroup\$ Some Improvements: 1. In checkCount() use $(".nextButton").toggle(count < 10); 2. Use Number as second param i.e. duration to animate(). Currently, strings are used '1200'. 3. Instead of changing color by using css(), create a CSS class and toggle it using toggleClass(). Same for visibility. 4. toggleVar is global. Avoid creating globals, wrap the code in IIFE. 5. Try to minimize the use of jQuery, use Angular. \$\endgroup\$ – Tushar Aug 10 '16 at 11:02
2
\$\begingroup\$

Depending on how the services work, I would, if possible, try to unify getAllJobs and searchJobs to a single service JobService and use this unified service.

Move ctrl.jobsList inside JobService, there is not particular reason why this has to be handled by the controller. Instead, you could just do a one to one ctrl.jobsList = JobService.jobsList in your controller. You will have to handle your concats in the service as well.

Move checkCount function to html using ng-show:

<div ng-click="jobs.getNext()" class="nextButton" ng-show="checkCount()">See More</div>

This will give you more opportunities to clean up your controller a little bit, maybe something like this:

angular.module('jobSeekerApp')
.controller('JobsallCtrl', ['JobService', function (JobService) {

    var ctrl = this;
    var count;

    ctrl.jobsList = JobService.jobsList;
    count = JobService.count;

    ctrl.pageNumber = 1;
    ctrl.searchPageNumber = 1;
    ctrl.isSearching = false;
    ctrl.searchTerm = "";

    JobService.getJobs(ctrl.pageNumber);

    ctrl.getNext = function() {

        if(isSearch) {
            ctrl.searchPageNumber = ctrl.searchPageNumber + 1;
            JobService.searchJob(ctrl.searchPageNumber, ctrl.searchTerm);
            return;
        }

        ctrl.pageNumber = ctrl.pageNumber + 1;
        JobService.getJobs(ctrl.pageNumber);

    };

    ctrl.search = function() {
        ctrl.searchTermLen = ctrl.searchTerm.length;

        if(isSearch) {
            ctrl.isSearching = true;
            ctrl.searchPageNumber = 1;
            JobService.searchJob(ctrl.searchPageNumber, ctrl.searchTerm);
            return;
        }

        ctrl.pageNumber = 1;
        JobService.getJobs(ctrl.pageNumber);
    };

    ctrl.deleteTerm = function (event) {

        if (event.keyCode === 8) {
            ctrl.searchTermLen = ctrl.searchTermLen - 1;
        }

        ctrl.isSearching = ctrl.searchTermLen !== 0;
    };

    ctrl.checkCount = function() {
        var result = count >= 10;
        count = count - 10;
        return result;
    };

    function isSearch() {
        return !(ctrl.searchTerm === "" && ctrl.isSearching === false);
    }

}]);

HEADS UP! This is just example code, untested.

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