Use array reduction when cleaning paragraphs #906
Conversation
This is more robust if the list of content tags is ever changed, since we no longer need to add terms to totalCount
| // We use an array reduction here so the counts of elements are summed | ||
| // without anyone having to make further code edits if the list of | ||
| // content tags is changed. | ||
| var totalCount = contentTags.reduce(function (total, tag) { | ||
| return total + paragraph.getElementsByTagName(tag).length; | ||
| }, 0); |
There was a problem hiding this comment.
So I'm a little lost. Any reason not to use this._getAllNodesWithTag(paragraph, contentTags).length ? That would seem easier than hand-rolling the reduce.
The other option, for reasons of speed, would be to iterate over contentTags (without a nested function) and to return false as soon as we find any of the content tags (as the return below will be false as soon as we find any of the elements in question). It might also make sense to put the _getInnerText call first in that case.
Both of those would still have the same benefit in terms of not requiring manual copy-pasting for each kind of inner tag that we're looking for.
There was a problem hiding this comment.
I see what you mean: I fixed one problem of weird methodology in the original, but not the other. I'll iterate further when I find some time.
The current code for cleaning non-content paragraphs is straightforward, but brittle against change: one must remember to modifiy both the list of element counts as well as
totalCount. This change removes the need to modifytotalCount(or indeed to set any more variables) by instead having a list of tag namesreduce()d intototalCountin one operation. It's slightly less readable, but it seemed worth it to me.