noobaa / noobaa-old-old-old

NooBaa - Crowd Cloud
noobaa.com
1 stars 0 forks source link

croford recomendations from chapter 1 #164

Closed yuvaldim closed 10 years ago

guymguym commented 10 years ago

@yuvaldim Checking hasOwnProperty() in every loop makes the code cluttered, and it's only relevant when using prototype inheritance, which is not the case for all these cases. In general I think this loop+check pattern makes the code harder to read more than the protection it provides. If you really think that we should protect against future inheritance of these collections then I suggest to use _.each(function(x){}) as standard instead which is cleaner.

yuvaldim commented 10 years ago

Couple of points:

  1. I read _.each code. Didn't really understood all of it but not sure it provide the needed protection.
  2. I don' think it affects code clutter in any way. Sure - another line of code - but why does it matter? Why is it less readable once you understand it is the best practice in this for x in y pattern?
  3. I fully agree and understand that none of the cases I changed required the change. That is exactly the point. One doesn't know if it was omitted on purpose or because they forgot. As it a best best practice, it's better to adapt in all cases and not think of it.

On Sat, Oct 4, 2014 at 2:29 AM, Guy notifications@github.com wrote:

@yuvaldim https://github.com/yuvaldim Checking hasOwnProperty() in every loop makes the code cluttered, and it's only relevant when using prototype inheritance, which is not the case for all these cases. In general I think this loop+check pattern makes the code harder to read more than the protection it provides. If you really think that we should protect against future inheritance of these collections then I suggest to use _.each(function(x){}) as standard instead which is cleaner.

— Reply to this email directly or view it on GitHub https://github.com/noobaa/noobaa/pull/164#issuecomment-57880622.

smile, you don't have much left La-Dimnik ;)

yuvaldim commented 10 years ago

.each is probably safe BUT it introuces a bigger change then I intended. As that is the case I'll merge what I've done. From this point on - for (x in y) should use .each...