Path: csiph.com!usenet.pasdenom.info!weretis.net!feeder4.news.weretis.net!news.teledata-fn.de!newsfeed.arcor.de!newsspool4.arcor-online.net!news.arcor.de.POSTED!not-for-mail Content-Type: text/plain; charset="UTF-8" Message-ID: <1699591.rPLLIT5s0r@PointedEars.de> From: Thomas 'PointedEars' Lahn Reply-To: Thomas 'PointedEars' Lahn Organization: PointedEars Software (PES) Date: Wed, 06 Mar 2013 00:03:13 +0100 User-Agent: KNode/4.8.5 Content-Transfer-Encoding: 8Bit X-Face: %i>XG-yXR'\"2P/C_aO%~;2o~?g0pPKmbOw^=NT`tprDEf++D.m7"}HW6.#=U:?2GGctkL,f89@H46O$ASoW&?s}.k+&. <2342178.BYbISWzQ9U@PointedEars.de> MIME-Version: 1.0 Lines: 218 NNTP-Posting-Date: 06 Mar 2013 00:03:14 CET NNTP-Posting-Host: e2ccf4fd.newsspool3.arcor-online.net X-Trace: DXC=DRQDZm8W4\YJN\b@mF9jNikdU3>YBYU3JEo]`[IdXSEdDIS X-Complaints-To: usenet-abuse@arcor.de Xref: csiph.com comp.lang.javascript:18725 Cezary Tomczyk wrote: > W dniu 2013-03-05 17:55, Thomas 'PointedEars' Lahn pisze: >> Cezary Tomczyk wrote: >>> Live example: http://jsfiddle.net/rFRwp/ >> >> This is Usenet, not the Web. An URI to a test case can be helpful, but >> it does not suffice. > > I know that this is Usenet, not the Web. I thought that code is too big > and instead of posting code here it would be better to prepare a working > example. You have posted 75 lines now. Of those, unless I miscounted or miscalculated, 48 of them are source code. I do not think that is “too big” for source code on this newsgroup. Consider that there have been discussions here with very valuable postings of 200 lines and more. Although few if any of those postings contained source code in all or even the majority of lines, what is to discuss *here* if we adopt a rule that, say, posting code was inappropriate when it exceeded 40 lines? There is an individual limit as to the acceptable length of a posting, but I doubt it starts below 100 lines for anyone. The same is true for source code in postings. This posting is likely to have more than 100 lines, including source code. What is usually frowned upon is posting uncommented source code that is not self-explanatory, and maybe even badly formatted, because the poster did not take care to understand it in the first place. Your code is neither. >>> Unfortunately, doesn't work yet. Am I going into right way? Or there is >>> better methods to clean up HTML? >> >> Perhaps. I cannot know for sure because I cannot scroll your script code >> on the Web, and you have not posted any code here. > > Here's the code: > > var is_block_style = function (el) { > var el_style; > if (window.getComputedStyle) { You should feature-test this method in a way that does not primarily use implicit type conversion. There is one DOM implementation, MSHTML, that is notorious for throwing strange exceptions on properties of type "unknown" otherwise. See also, e.g., jsx.object.isHostMethod(). > el_style = window.getComputedStyle(el, > null).getPropertyValue('display'); > } else { > el_style = el.currentStyle.display; > } > return el_style; > }; You should generalize this code, see also jsx.dom.getComputedStyle(). In this case, because the base object always is the same, you can even safely perform the branching for the window.getComputedStyle() DOM method – which should be document.defaultView.getComputedStyle() to be compatible – only once. You cannot assume, however, that if the getComputedStyle() method is absent, the “currentStyle“ property must be present; that is an inductive fallacy (hasty generalization). A method whose name starts with “is” should not return a string value, but a boolean value. In this case, it should return the result of the comparison against any of those property values which should be considered indicative of a “block element” (see CSS 2.1, also CSS3 Box Model for details). > var imgClassName = 'testimg'; > > var tidyHTML = function (node) { > var elms = node.childNodes, > i = elms.length, > text = '', > elm; > > if (!node.hasChildNodes() I have never used that method before. Unless you are sure about its backwards-compatibility, you should replace the call with if (!node.childNodes.length > 0 If you do not insist on an explicit condition, use if (!node.childNodes.length or if (!node.firstChild whereas the latter is probably the more efficient variant of the two. I think it is safe to assume that those values can be type-converted if necessary, because otherwise a lot of simple client-side scripts would fail. But if necessary and appropriate for your use-case, use if (null != node.firstChild or if (node.firstChild !== null In theory, if (null !== node.firstChild should have the best runtime efficiency, returning in step 3 of the “Strict Equality Comparison Algorithm” (ES 5.1, 11.9.6) if node.firstChild is null, and returning in step 1 if it is not, already. (I have not applied this knowledge yet. Profiling will show if I should.) > && (node.nodeName.toLowerCase() === 'img' > && !(new RegExp('(^|\\s)' + imgClassName + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > '(\\s|$)')).test(node.className))) { ^^^^^^^^^^^ You should move or even generalize this code in order to leverage caching. You can do that because the RegExp does not have the “global” flag set (i.e., its boolean ”global” property has the value “false”). > node.parentNode.removeChild(node); > return text; > } > > while (i--) { > elm = elms[i]; So AIUI you iterate backwards through the child nodes … > if (elm.nodeType === 1) { A strict comparison does not appear to be necessary or prudent here. You cannot know that the types match since you are dealing with a host object. What you can know (from the ECMAScript Specification) is that *if* the types match, it does not matter for the result or efficiency if you use a strict comparison or not. And you gain a little bit more backwards-compatibility that way. > text += tidyHTML(elm); … but why do you *append* to the “text” variable as if you had iterated forwards, thereby reversing the order of the content? Equivalent code: var a = ["foo", "bar", "baz"]; var i = a.length; var a2 = []; while (i--) { if (a[i] != "bar") { a2.push(a[i]); } } /* ["baz", "foo"] */ console.log(a2); You are looking for something more like for (var i = 0, len = elms.length; i < len; ++i) { text += tidyHTML(elms[i]); } > } else if (elm.nodeType === 3) { > text += elm.data; > } else if (elm.nodeType !== 3 && elm.nodeType !== 1) { > elm.parentNode.removeChild(elm); > } ISTM that at least one of those should happen before the loop, and before the function returns, in the trivial case. Pseudo-code: function recurse (target) { if (isTrivial(target)) { return trivialValue; } for (var property in target) { recurse(target[property]); } return nonTrivialValue; } > } > > if (typeof text !== 'undefined') { Again an unnecessary strict comparison. > if (is_block_style(node) === 'block') { > node.parentNode.insertBefore(document.createElement('br'), > node.nextSibling); > } Why? Usually it is misuse of the “br” element to use it as replacement for a block delimiter. Also, “display: block” does not necessarily mean that there is a paragraph. Consider, for example, positioned or floating block elements. > //node.parentNode.replaceChild(document.createTextNode(text), > node); > } > return text; > }; And BTW, “doesn't work (yet)” is not a problem description. -- PointedEars Twitter: @PointedEars2 Please do not Cc: me. / Bitte keine Kopien per E-Mail.