4 ms·

Well the second snippet hides the loop and conditional which surely are implemented in "Where". Is that a code smell by proxy?

by sidlls 10y ago

krylon 10y ago
In a way, yes. On the other hand, it boils down the code that some poor soul of a maintenance programmer needs to deal with to a single line which is more readable, too. Win-Win.
- sidlls 10y ago
  I don't disagree. But then again I've had peers review code similar to this and flag an issue saying it's too clever because that poor soul might not know what "where" is doing or that the use of lambdas is "too complex."
  - PJDK 10y ago
    That is certainly something I've also come across. This kind of functional style is very readable once you understand it, but it does have a learning curve just like learning what "for(var i = 0; i< length; i++)" means. I actually did a little presentation to the team about how to refactor a for loop to an self implemented where/filter function to explain the connection. For me, realising that all the where was doing (or at least potentially, there might be some behind the scenes optimisations going on) was passing in the condition for an IF in a FOR loop suddenly clarified everything for me. In case anyone's interested, this is these are the code steps (javascript) var people = [{name: 'bob', gender: 'male'}, {name: 'dave', gender: 'male'}, {name: 'sarah', gender: 'female'}]; var getAllMen = function(population) { var returnList = []; for(var i = 0; i< population.length; i++){ if(population[i].gender === 'male'){ returnList.push(population[i]); } } return returnList; } var males = getAllMen(population); ___ var getAllMen = function(population) { var returnList = []; for(var i = 0; i< population.length; i++){ if(isMan(population[i]){ returnList.push(population[i]); } } return returnList; } var isMan = function(person){ return person.gender === 'male'; } var males = getAllMen(population); ___ var filter = function(anyOldList, filterCondition){ var returnList = []; for(var i = 0; i< anyOldList.length; i++){ if(filterCondition(anyOldList[i]){ returnList.push(anyOldList[i]); } } return returnList; } var isMan = function(person){ return person.gender === 'male'; } var males = filter(population, isMan); var females = filter(population, function(person){ person.gender === 'female'; }); var females = filter(population, person => person.gender === 'female'); var females = filter(population, x => x.gender === 'female');
lmm 10y ago
Depends which expresses the actual intent more clearly. To my mind, that it's the list of foos that are good is what's important, and the loop and conditional are implementation details.
kosievdmerwe 10y ago
Here's one thing to keep in mind. They both end up doing the same thing, but it's more obvious that the Where implementation is not doing anything subtly different from what you think at first glance. Without reading the for implementation carefully it's much harder to verify: * You're looping from start to finish * Over each element * You're only adding the items to the new list that match the conditions. That all said, I think this swap is only a good idea if the thing you're swapping with is commonly used. Which admittedly is a chicken-and-egg problem, but does largely exclude non-library function transformations or one-off transformations.

Well the second snippet hides the loop and conditional which surely are implemented in "Where". Is that a code smell by proxy?

by sidlls 10y ago

Well the second snippet hides the loop and conditional which surely are implemented in "Where". Is that a code smell by proxy?

krylon 10y ago

In a way, yes. On the other hand, it boils down the code that some poor soul of a maintenance programmer needs to deal with to a single line which is more readable, too. Win-Win.

sidlls 10y ago
I don't disagree. But then again I've had peers review code similar to this and flag an issue saying it's too clever because that poor soul might not know what "where" is doing or that the use of lambdas is "too complex."
- PJDK 10y ago
  That is certainly something I've also come across. This kind of functional style is very readable once you understand it, but it does have a learning curve just like learning what "for(var i = 0; i< length; i++)" means. I actually did a little presentation to the team about how to refactor a for loop to an self implemented where/filter function to explain the connection. For me, realising that all the where was doing (or at least potentially, there might be some behind the scenes optimisations going on) was passing in the condition for an IF in a FOR loop suddenly clarified everything for me. In case anyone's interested, this is these are the code steps (javascript) var people = [{name: 'bob', gender: 'male'}, {name: 'dave', gender: 'male'}, {name: 'sarah', gender: 'female'}]; var getAllMen = function(population) { var returnList = []; for(var i = 0; i< population.length; i++){ if(population[i].gender === 'male'){ returnList.push(population[i]); } } return returnList; } var males = getAllMen(population); ___ var getAllMen = function(population) { var returnList = []; for(var i = 0; i< population.length; i++){ if(isMan(population[i]){ returnList.push(population[i]); } } return returnList; } var isMan = function(person){ return person.gender === 'male'; } var males = getAllMen(population); ___ var filter = function(anyOldList, filterCondition){ var returnList = []; for(var i = 0; i< anyOldList.length; i++){ if(filterCondition(anyOldList[i]){ returnList.push(anyOldList[i]); } } return returnList; } var isMan = function(person){ return person.gender === 'male'; } var males = filter(population, isMan); var females = filter(population, function(person){ person.gender === 'female'; }); var females = filter(population, person => person.gender === 'female'); var females = filter(population, x => x.gender === 'female');

lmm 10y ago

Depends which expresses the actual intent more clearly. To my mind, that it's the list of foos that are good is what's important, and the loop and conditional are implementation details.

kosievdmerwe 10y ago

Here's one thing to keep in mind. They both end up doing the same thing, but it's more obvious that the Where implementation is not doing anything subtly different from what you think at first glance. Without reading the for implementation carefully it's much harder to verify: * You're looping from start to finish * Over each element * You're only adding the items to the new list that match the conditions. That all said, I think this swap is only a good idea if the thing you're swapping with is commonly used. Which admittedly is a chicken-and-egg problem, but does largely exclude non-library function transformations or one-off transformations.