Angular controller without single responsability

I'm new in cordova / angularJS and I wonder how much responsibility a controller can have. This particular one validates users entry data, display messaging, takes pictures calling the camera API, serialize them and finally calls a service to upload them to the cloud. I want to refactor functionality and move it to a separate service for example but not sure which functions should be moved.

```
.controller('pictureOfTheVehicleCtrl', function ($scope, $cordovaCamera, $rootScope, $ionicPopup, Upload, $ionicLoading, $timeout) {

$scope.upload = function () {
// Validate user input
var message = undefined;
if ($scope.registrationNumber === undefined)
message = 'Please, provide a valid registration number.';
else if ($scope.damage === undefined)
message = "Please, provide a valid picture of the damage. ";
else if ($scope.vehicle === undefined)
message = "Please, provide a valid vehicle picture. ";

if (message) {
$ionicLoading.show({ template: message });
$timeout(function () {
$ionicLoading.hide();
}, 2000);
return null;
}

// Upload damage picture
var file = dataURItoBlob($scope.damage);
$ionicLoading.show({ template: 'Uploading picture of the damage...' });
Upload.uploadImage($rootScope.userName, $rootScope.userPass, $scope.registrationNumber, 'damage', file)
.then(function (response) { // success
}, function (response) { // error
showAlert('Error', 'Error uploading picture of the damage: ' + response)
return null;
}).then(function () {
$ionicLoading.hide();
});

// Upload vehicle picture
file = dataURItoBlob($scope.vehicle);
$ionicLoading.show({ template: 'Uploading vehicle picture...' });
Upload.uploadImage($rootScope.userName, $rootScope.userPass, $scope.registrationNumber, 'licenseplate', file)
.then(function (response) { // success

You should export any code that can be used by multiple controller to a service or a utility. This will prevent code repetition, for example

Upload function

Upload.uploadImage($rootScope.userName, $rootScope.userPass, $scope.registrationNumber, 'damage', file)
  .then(function(response) { // success
  }, function(response) { // error
    showAlert('Error', 'Error uploading picture of the damage: ' + response)
    return null;
  }).then(function() {
    $ionicLoading.hide();
  });
}

You are calling this function twice and most of structure is similar. This can be made in a single function and called twice.

Note: Not the best example but can be made generic.

function uploadImage(tag, file, success) {
  Upload.uploadImage($rootScope.userName, $rootScope.userPass, $scope.registrationNumber, tag, file)
    .then(success, function(response) { // error
      showAlert('Error', 'Error uploading picture of the vehicle: ' + response)
      return null;
    }).finally(function() {
      $timeout(function() {
        $ionicLoading.hide();
      }, 2000);
    });
}

uploadImage('damage', file);
uploadImage('licenseplate', file, function(response){
   if (response == 0) {
        showAlert('Upload', 'Pictures successfully uploaded!')
        wipeData();
      } else {
        showAlert('Error', 'Error uploading picture of the vehicle: ' + response)
      }
});

Validate Function

You should export all validation code to another function and just call this function.

function ValidateInput() {
  // Validate user input
  var message = undefined;
  if ($scope.registrationNumber === undefined)
    message = 'Please, provide a valid registration number.';
  else if ($scope.damage === undefined)
    message = "Please, provide a valid picture of the damage. ";
  else if ($scope.vehicle === undefined)
    message = "Please, provide a valid vehicle picture. ";

  return message;
}

$scope.upload = function() {
  var message = validateInput();
  if (message) {
    $ionicLoading.show({
      template: message
    });
    $timeout(function() {
      $ionicLoading.hide();
    }, 2000);
    return null;
  }

Password

In this call,

Upload.uploadImage($rootScope.userName, $rootScope.userPass, $scope.registrationNumber, tag, file)

userPass should not be stored in a rootScope. Ideally, it should be saved on server and in encrypted format and using a one time random key and this encrypted value can be stored.

StackExchange Code Review Q#114387, answer score: 2