2
\$\begingroup\$

I'm an 'old school' programmer (C, C++), and I have step-by-step, algorithmic thinking. I'm writing some code in Angular + Django and I know that this code is ugly but I don't have any idea how to refactor it.

First of all, I have the models:

class Product(models.Model):
    name = models.CharField(max_length=100)
    status = models.CharField(max_length=10, choices=STATUSES.items(), default='new')

class Ad(models.Model):
    image = models.ImageField(upload_to="ad")
    width = models.IntegerField(blank=True)
    height = models.IntegerField(blank=True)
    product = models.ForeignKey(Product, null=True, blank=True)

class AdStatus(models.Model):
    status = models.IntegerField(default=1)
    ad = models.ForeignKey(Ad)

Next I have simple classes for serializing:

class AdSerializer(serializers.HyperlinkedModelSerializer):
    class Meta:
        model = Ad
        fields = ["url", "id", "image", "width", "height". "product"]
#etc...

And every model has its own factory in Angular:

  .factory('AdStatus',["$resource", function ($resource){
    var Resource = $resource(
        "/api/ad_status/:ad_id/",
        {ad_id: '@id'},
        {
            query: {
                isArray: true,
                transformResponse: function (data) {
                    var items = angular.fromJson(data);
                    return items.results;
                }
            },
            update: {
                method: "PUT",
            }
        },
        {
            stripTrailingSlashes: false
        }
    );
    Resource.prototype.changeStatus = function(status){
        this.status = status;
        this.$update();
    };
    return Resource;
  }])
/etc

This is the main code:

//controller isn't bad

angular.module('controllers.statuses', [])
.controller('StatusCtr', ["$scope", "$stateParams", "StatusSetter",  function($scope, $stateParams, StatusSetter) {
    $scope.click = function(){
        $scope.t = StatusSetter.change($stateParams.id);// this is id of product
    };
}]);

But this makes me a little sick:

.factory('StatusSetter', ["AdStatus", "Product", "Ad", function(AdStatus, Product, Ad){
    var changeStatus = function(id, productStatus, adStatus){
        var product = Product.get({product_id: id});
        product.$promise.then(function(data) {
            product.changeStatus(productStatus);
            var ads = Ad.query();
            var ad_status = AdStatus.query();
            var tmp = [];
            ads.$promise.then(function(data){
                for (var i = 0; i < ads.length; i++) {
                    if (ads[i].product == product.url){
                        tmp.push(ads[i]);
                    }
                }
                ad_status.$promise.then(function(data){
                    for (var i = 0; i < ad_status.length; i++){
                        for (var j = 0; j < tmp.length; j++){
                            if (ad_status[i].ad == tmp[j].url){
                                ad_status[i].changeStatus(adStatus);
                            }
                        }
                    }

                })
            });
        });
        return product;
    };
    return {
        change: function(id){
            return changeStatus(id, "new_status", 30);
        }
    }
  }])

The main philosophy of this code is:

change AdStatuses which points at Ads (which points at Products)

Any ideas? Maybe I can separate this 'filtering' to $resource or something.

\$\endgroup\$
1
  • \$\begingroup\$ Welcome to Code Review! The title of your post should contain the function or purpose of your code. \$\endgroup\$ – SirPython Nov 12 '15 at 20:25
2
\$\begingroup\$

I think what you want to do is:

  • Get the product URL
  • Find the Ads with that product URL
  • Use each Ad URL to find the relevant AdStatus object
  • Update each AdStatus object

This can be done by chaining the operations with the $resource.$promise

.factory('StatusSetter', ["AdStatus", "Product", "Ad", "$q" 
                          function(AdStatus, Product, Ad, $q)
{
    var changeStatus = function(id, newStatus, newValue){
        var filteredAdsList = [];
        var product = Product.get({product_id: id});
        product.$promise.then(function(productObj) {
            //chain product status change
            return productObj.changeStatus(newStatus).$promise;
        }) .then (function (productObj) {
            //wait for product status to finish change
            //chain Ad query
            return  Ad.query().$promise;
        }) .then (function (adsObj) (
            //push filtered list
            angular.forEach(adsObj, function (iAd)) {
                 if (iAd.product == product.url){
                     filteredAdsList.push(iAd);
                 }
             };
            //chain adStatus query 
            return AdStatus.query().$promise;
        }) .then (function (adStatusObj) {})
            //start with empty promise
            var qPromise = $q.when();
            angular.forEach(adStatusObj, function (iAdStatus) {
                angular.forEach(filteredAdsList, function(jAd) {
                     if (iAdStatus.ad == jAd.url){
                         //add to promise chain
                         qPromise = qPromise.then (function() {
                             var nextPromise =
                                iAdStatus.changeStatus(newValue).$promise;
                             return nextPromise;
                         });
                     };
                });
            });
            //return promise chain
            return qPromise;
        }) .then (function () {
            console.log("all status updated")
        })
        return product;
    };
    return {
        change: function(id){
            return changeStatus(id, "new_status", 30);
        }
    }
}])

By returning data and promises and chaining those promises, you can create code that uses asynchronous APIs in a way that looks familiar.

For more information on the AngularJS $q service and chaining promises see AngularJS $q Service API Reference - chaining promises

Also notice that I used angular.forEach for iteration. For more information on using angular.forEach see the AngularJS angular.forEach API Reference.

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