3

I'm already using an Array.map, but with the same data, I need to do some other calculations. Should I do those calculations within the map or do it in a for each after the map?

    return res.data.map(function (obj) {
        if(obj.status.id == 6 || obj.status.id == 5){
            dateDifference(obj.created_at,obj.closed_at);
        }else{
            $scope.open++;
        }
        return {
            "id":obj.id,
            "subject": obj.subject,
            "requester": obj.requester.name,
            "assigned": obj.assigned ? obj.assigned.name : '',
            "priority": obj.priority.name,
            "status": obj.status.name,
            "category": obj.category.name,
            "created_at": moment(obj.created_at).utcOffset("06:00").format('lll'),
            "updated_at": moment(obj.updated_at).utcOffset("06:00").format('lll')
        }
     })
Jason Spick
  • 6,028
  • 13
  • 40
  • 59
  • 2
    I'd argue Array.map, being declarative, shouldn't have any side effects, but that's just my two cents. – Jack Guy Feb 24 '16 at 16:10
  • 1
    it depends..... post an example. – Daniel A. White Feb 24 '16 at 16:10
  • 1
    seriously got -2. I researched this and there was no good answer. – Jason Spick Feb 24 '16 at 16:15
  • 2
    The downvotes are likely because you didn't specify what "some other calculations" are exactly. As so often, details matter. Then, overall, this seems more of a subjective question. – Felix Kling Feb 24 '16 at 16:17
  • Thank you for the feedback. There is really no goo material out there on this, I wasn't sure how to ask the question. – Jason Spick Feb 24 '16 at 16:25
  • `dateDifference` doesn't sound like it would have side effects. Why are you calling it at all? – Bergi Feb 24 '16 at 16:25
  • What would be an instance where there would be a side effect. – Jason Spick Feb 24 '16 at 16:26
  • Yes, given that there is not even a benefit (such a sharing calculations) between the side effects and the creation of the new array values, you should definitely separate them. – Bergi Feb 24 '16 at 16:26
  • @JasonSpick `$scope.open++` is a side effect. – Bergi Feb 24 '16 at 16:27
  • 1
    Whether the callback shouldn't have side effects or not is debatable. The way the [MDN docs on `map`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/map) are written, it's expected that even the array that `map` is being called on will be modified at certain points, never mind other side effects. [And the spec](http://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.map), for that matter: _"map does not directly mutate the object on which it is called but the object may be mutated by the calls to callbackfn."_ – James Thorpe Feb 24 '16 at 16:34

1 Answers1

1

I don't see it as a 'good practice', but I don't see it as a bad practice either. Fact is, .map() is designed to both iterate and create a new resulting array.

Perhaps the answer lies in its definition.

From the JS website :

"The map() method creates a new array with the results of calling a provided function on every element in this array."

From the PHP website (for fun) :

"array_map() returns an array containing all the elements of array1 after applying the callback function to each one."

Nothing keeps you from doing what you want in that callback function.

If you, personnally, are not comfortable doing that, you could achieve the same thing doing a 'for each' while building your own new array. Which is probably what I would do in your specific case. I'd rather do that than have to iterate 2 times over my array.

Though, as Bergi mentionned, it is a better practice to iterate twice over the array if it makes sense sematically.

If performance were to become an issue (iterating twice on a long array). Here is what I would be inclined to do :

for (var i=0; i<myArray.length; i++) {
    doStuffA(myArray[i]);
    doStuffB(myArray[i]);
}

which is quite clear semantically. Instead of iterating twice.

Of course, some pleople might (will probably) disagree with me.

phenxd
  • 689
  • 4
  • 17
  • Nice. That was my reason for asking. Not having 2 iterations. But I like your though on using a foreach instead of a map and just creating a new array. – Jason Spick Feb 24 '16 at 16:51
  • "*I'd rather do that than have to iterate 2 times over my array.*" - there's nothing wrong with iterating twice. If it makes sense semantically, you should do that. – Bergi Feb 24 '16 at 16:52
  • I agree. I was giving my opinion about the specific mentionned case. – phenxd Feb 24 '16 at 16:56
  • If you cared about performance (or about anything), [you wouln't use `for…in` enumerations on arrays!](https://stackoverflow.com/q/500504/1048572) :-) – Bergi Feb 24 '16 at 17:06
  • I do care about some things... :/ haha. Thanks for the info, I'll edit! – phenxd Feb 24 '16 at 17:42