HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Property management

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
managementpropertystackoverflow

Problem

I was looking to see if anyone could help me come up with a cleaner/DRY methods in duplicate of doing something like this?

I would like to get the features that repeats in the 2 controllers to generalize my code.

```
var propertiesModule = angular.module('app.properties', []);

propertiesModule.controller('PropertiesCtrl', ['$scope', '$routeParams', 'PropertiesService', 'ApplicationService', 'PlatformService', 'Page', function ($scope, $routeParams, PropertiesService, ApplicationService, PlatformService, Page) {

Page.setTitle("Properties");
$scope.platform = $routeParams.platform;

$scope.platforms = [];

$scope.on_edit_platform = function (platform_name) {
/ Reset unit choice /
$scope.unit = undefined;
};

$scope.add_platform = function (platform_name) {
if (!_.contains($scope.platforms, platform_name)) {
$scope.platforms.push(platform_name);
}
};

$scope.delete_platform = function (platform) {
//Might be a bit tricky
};

$scope.on_edit_unit = function (unit) {

PropertiesService.getPropertiesMergedWithModel("properties#" + $scope.application.name + "#" + $scope.application.version + "#" + $scope.platform + "#" + unit.name, unit.modelNamespaces).then(function (properties) {
$scope.properties = properties;

});
};

$scope.save_properties = function (properties) {
PropertiesService.save(properties).then(function (properties) {
PropertiesService.getModel($scope.unit.modelNamespaces).then(function (model) {
$scope.properties = properties.mergeWithModel(model);
});
});
};

$scope.on_concat_plateformes = function () {
var a = $scope.platforms.concat($scope.platformsCible);
for (var i = 0; i b[property]) ? 1 : 0;
return result * sortOrder;
}
}

ApplicationService.get($routeParams.application, $routeParams.version).then(function (appli

Solution

A short, nitpicking review:

-
Here, there seems to be no good reason to choose key and value instead of name and value, it just seems like it makes things more complicated.

var tab1 = [];

        for (var i = 1; i <= $scope.properties.key_value_properties.length - 1; i++) {

            tab1.push({
                key: $scope.properties.key_value_properties[i].name,
                value: $scope.properties.key_value_properties[i].value
            });
        }

        var tab2 = [];
        for (var i = 1; i <= propertiesCible.key_value_properties.length - 1; i++) {
            tab2.push({
                key: propertiesCible.key_value_properties[i].name,
                value: propertiesCible.key_value_properties[i].value
            });
        }


could be

var tab1 = $scope.properties.key_value_properties,
            tab2 = propertiesCible.key_value_properties;


-
Also, mandatory bash on using French in variables names. Don't do it, propertiesCible -> propertiesTarget

-
Consider using syntax sugar (in this example using shortcut variables) and remove useless comments, this

$scope.properties.differences = [];
        $scope.properties.absentInTab1 = [];
        $scope.properties.presentInTab1 = [];

        for (var i in obj1) {
            if (typeof(obj2[i]) === 'undefined') {
                // presentInTab1.push({ key: i, oldValue: obj1[i], newValue: null });
                $scope.properties.presentInTab1.push({name: i, valueRef: obj1[i], valueCible: null});
            }
            else if (obj1[i] !== obj2[i]) {
                //differences.push({ key: i, oldValue: obj1[i], newValue: obj2[i] });
                $scope.properties.differences.push({name: i, valueRef: obj1[i], valueCible: obj2[i]});
            }
        }
        for (var i in obj2) {
            if (typeof(obj1[i]) === 'undefined') {
                // absentInTab1.push({ key: i, oldValue: null, newValue: obj2[i] });
                $scope.properties.absentInTab1.push({name: i, valueRef: null, valueCible: obj2[i]});
            }
        }


could be

var differences = $scope.properties.differences = [],
            absentInTab1 = $scope.properties.absentInTab1 = [],
            presentInTab1 = $scope.properties.presentInTab1 = [];

        for (var i in obj1) {
            if (typeof(obj2[i]) === 'undefined') {
                presentInTab1.push({name: i, valueRef: obj1[i], valueCible: null});
            }
            else if (obj1[i] !== obj2[i]) {
                differences.push({name: i, valueRef: obj1[i], valueCible: obj2[i]});
            }
        }
        for (var i in obj2) {
            if (typeof(obj1[i]) === 'undefined') {
                absentInTab1.push({name: i, valueRef: null, valueCible: obj2[i]});
            }
        }


  • I strive to never use null but undefined instead



-
Be more ardent in reducing copy paste lines, the following seems a good example:

save: function (properties) {
    properties = properties.toAppEntity();
    if (properties.versionID < 0) {
        return $http.post('rest/properties/' + encodeURIComponent(properties.namespace), properties).then(function (response) {
            $.notify("Les proprietes ont bien ete crees", "success");
            return new Properties(response.data);
        }, function (error) {
            $.notify(error.data, "error");
        });
    } else {
        return $http.put('rest/properties/' + encodeURIComponent(properties.namespace), properties).then(function (response) {
            $.notify("Les proprietes ont bien ete mises a jour", "success");
            return new Properties(response.data)
        }, function (error) {
            $.notify(error.data, "error");
        });
    }
}


you could try this:

save: function (properties) {
    var createdMessage = 'Les proprietes ont bien ete crees',
        updatedMessage = 'Les proprietes ont bien ete mises a jour',
        message = properties.versionID < 0 ? createdMessage : updatedMessage;

    properties = properties.toAppEntity();
        return $http.put('rest/properties/' + encodeURIComponent(properties.namespace), properties).then(function (response) {
            $.notify( message  , "success");
            return new Properties(response.data)
        }, function (error) {
            $.notify(error.data, "error");
        });

}


-
You could reduce the vertical stretching in your code quite a bit by using _.pick():

return {
                name: kvp.name,
                comment: kvp.comment,
                value: kvp.value
            }


could be

return _.pick( kvp, 'name', 'comment', 'value');

Code Snippets

var tab1 = [];

        for (var i = 1; i <= $scope.properties.key_value_properties.length - 1; i++) {

            tab1.push({
                key: $scope.properties.key_value_properties[i].name,
                value: $scope.properties.key_value_properties[i].value
            });
        }

        var tab2 = [];
        for (var i = 1; i <= propertiesCible.key_value_properties.length - 1; i++) {
            tab2.push({
                key: propertiesCible.key_value_properties[i].name,
                value: propertiesCible.key_value_properties[i].value
            });
        }
var tab1 = $scope.properties.key_value_properties,
            tab2 = propertiesCible.key_value_properties;
$scope.properties.differences = [];
        $scope.properties.absentInTab1 = [];
        $scope.properties.presentInTab1 = [];

        for (var i in obj1) {
            if (typeof(obj2[i]) === 'undefined') {
                // presentInTab1.push({ key: i, oldValue: obj1[i], newValue: null });
                $scope.properties.presentInTab1.push({name: i, valueRef: obj1[i], valueCible: null});
            }
            else if (obj1[i] !== obj2[i]) {
                //differences.push({ key: i, oldValue: obj1[i], newValue: obj2[i] });
                $scope.properties.differences.push({name: i, valueRef: obj1[i], valueCible: obj2[i]});
            }
        }
        for (var i in obj2) {
            if (typeof(obj1[i]) === 'undefined') {
                // absentInTab1.push({ key: i, oldValue: null, newValue: obj2[i] });
                $scope.properties.absentInTab1.push({name: i, valueRef: null, valueCible: obj2[i]});
            }
        }
var differences = $scope.properties.differences = [],
            absentInTab1 = $scope.properties.absentInTab1 = [],
            presentInTab1 = $scope.properties.presentInTab1 = [];

        for (var i in obj1) {
            if (typeof(obj2[i]) === 'undefined') {
                presentInTab1.push({name: i, valueRef: obj1[i], valueCible: null});
            }
            else if (obj1[i] !== obj2[i]) {
                differences.push({name: i, valueRef: obj1[i], valueCible: obj2[i]});
            }
        }
        for (var i in obj2) {
            if (typeof(obj1[i]) === 'undefined') {
                absentInTab1.push({name: i, valueRef: null, valueCible: obj2[i]});
            }
        }
save: function (properties) {
    properties = properties.toAppEntity();
    if (properties.versionID < 0) {
        return $http.post('rest/properties/' + encodeURIComponent(properties.namespace), properties).then(function (response) {
            $.notify("Les proprietes ont bien ete crees", "success");
            return new Properties(response.data);
        }, function (error) {
            $.notify(error.data, "error");
        });
    } else {
        return $http.put('rest/properties/' + encodeURIComponent(properties.namespace), properties).then(function (response) {
            $.notify("Les proprietes ont bien ete mises a jour", "success");
            return new Properties(response.data)
        }, function (error) {
            $.notify(error.data, "error");
        });
    }
}

Context

StackExchange Code Review Q#69744, answer score: 2

Revisions (0)

No revisions yet.