Board Thread:Code Review/@comment-1757994-20151023054248 — dev

DupImageList review | Fandom

ALL POSTS

Saftzie·10/23/2015 in JS Development

DupImageList review

(Edited by Saftzie)

DupImageList

Saftzie·10/24/2015

<p>I ran both versions of the code with the console open and a counter inserted at the jQuery getJSON statement. </p><p>In both versions, getJSON executed 77 times at Community Central. </p><p>The original code died after the 77th request
uncaught exception: TypeError: data['query-continue'].allimages is undefined
just as expected when dfcontinue exists and gaifrom doesn't. </p><p>I didn't measure the traffic volume. </p>

(Edited by Saftzie)

Dessamator·10/24/2015

<p>Well, it seems the code has more bugs than initially thought. It was also developed a long time ago, so a fresh new version is a good thing. But it would be better to hold off on that until the performance can be improved. </p><p>Edit </p><p>I think you should just show the first batch of the results, and store the rest. One the requests are done, then simply drop everything else. As far as improving the number of requests, it is not that big of a deal. it is just around 200kb more. Considering that if one uses that scripts frequently it will never become laggy because all duplicates will be deleted every so often. </p>

(Edited by Dessamator)

Saftzie·10/24/2015

<p>I described all these bugs in the opening post. </p>

The original code doesn't find all the duplicates
Some of the duplicates that it does find it lists multiple times
It errors out on the last iteration

<p>I'm not sure what you mean by improving performance. I got 77 requests for both the original code and the updated code on Community Central (as opposed to 135 and 234). There would be a bit more network traffic, because the updated code is actually having the API return information on all the duplicate files, instead of only some of them. 30% more may still be accurate. There's no way to reduce the network traffic, because there's no way to tell allimages to skip files already known to be duplicates or to skip files that have no duplicates. (That second point is a serious annoyance with the API, but it is what it is.) All the other information the API returns is necessary. </p><p>As for batching the results, do you mean doing one large DOM operation at the end instead of one DOM operation for each file with duplicates? If so, I considered it, but it's not how the original code works, and I didn't want to change what the user sees as it runs. The intent of the review/update is to fix bugs only. Plus, if something causes the connection between the browser to the server to glitch, the user has at least a partial list, instead of a blank screen. </p>

(Edited by Saftzie)

Dessamator·10/24/2015

<p>Yes, fixing the bugs is fine. I originally meant that with this script one never really knows when it is done. So if one has a laggy connection it may be a problem. </p><p>I meant something like this: </p> <div dir="ltr" class="mw-geshi mw-content-ltr"><div class="javascript source-javascript">

<span class="kw1">var</span> remainingResults <span class="sy0">=</span> <span class="st0">""</span><span class="sy0">;</span>
<span class="kw1">var</span> isFirst <span class="sy0">=</span> <span class="kw2">true</span><span class="sy0">;</span>
 
<span class="kw1">if</span> <span class="br0">(</span>output<span class="br0">)</span> <span class="br0">{</span>
   <span class="kw1">if</span> <span class="br0">(</span>isFirst<span class="br0">)</span> <span class="br0">{</span>
        $<span class="br0">(</span><span class="st0">'#mw-dupimages'</span><span class="br0">)</span>.<span class="me1">append</span><span class="br0">(</span>output <span class="sy0">+</span> <span class="st0">'</ul>'</span><span class="br0">)</span><span class="sy0">;</span>
         isFirst <span class="sy0">=</span> <span class="kw2">false</span><span class="sy0">;</span>
    <span class="br0">}</span>
    <span class="kw1">else</span><span class="br0">{</span>
    	output <span class="sy0">+=</span> <span class="st0">"more output"</span>
    <span class="br0">}</span>   
<span class="br0">}</span>
<span class="co1">// do other stuff</span>
<span class="co1">// when processing ends</span>
 
<span class="kw1">if</span> <span class="br0">(</span>output<span class="br0">)</span> <span class="br0">{</span>
	$<span class="br0">(</span><span class="st0">'#mw-dupimages'</span><span class="br0">)</span>.<span class="me1">append</span><span class="br0">(</span>output <span class="sy0">+</span> <span class="st0">'</ul>'</span><span class="br0">)</span><span class="sy0">;</span>
<span class="br0">}</span>

</div></div> <p>Maybe adding some kind of indicator that stuff is still happening in the background would be nice too. </p><p>It is a minor change, so I don't think people will mind. </p>

(Edited by Dessamator)

Saftzie·10/24/2015

<p>There is the spinner on the right, though it's not particularly noticeable to me. We could move it to the left, I suppose. </p><p>Amusingly there are talk page comments from a while ago about how the spinner kept spinning after the code exited. That was probably because the code died instead of exiting normally, but the fix at the time was to move hiding the spinner to before the point where the error caused the code to die. </p>

(Edited by Saftzie)

Saftzie·10/25/2015

<p>Something like this is what I meant, although it's not really a bug. </p><p>
</p>

Edit

	$<span class="br0">(</span><span class="st0">'#dupImagesProgress'</span><span class="br0">)</span>.<span class="me1">show</span><span class="br0">(</span><span class="br0">)</span><span class="sy0">;</span>
	<span class="kw1">var</span> indicator <span class="sy0">=</span> stylepath <span class="sy0">+</span> <span class="st0">'/common/progress-wheel.gif'</span><span class="sy0">,</span>
	url <span class="sy0">=</span> <span class="st0">'/api.php?format=json&action=query&prop=duplicatefiles&dflimit=500&generator=allimages&gailimit=500'</span><span class="sy0">;</span>
	dil <span class="sy0">=</span> dil <span class="sy0">||</span> <span class="br0">{</span><span class="br0">}</span><span class="sy0">;</span>
 
	<span class="kw1">if</span> <span class="br0">(</span><span class="sy0">!</span><span class="br0">(</span>$<span class="br0">(</span><span class="st0">'#dupImagesProgress'</span><span class="br0">)</span>.<span class="me1">length</span><span class="br0">)</span><span class="br0">)</span> <span class="br0">{</span>
		$<span class="br0">(</span><span class="st0">'#mw-dupimages'</span><span class="br0">)</span>.<span class="me1">prepend</span><span class="br0">(</span><span class="st0">'<span style="float: right;" id="dupImagesProgress" title="In progress..."><img src="'</span> <span class="sy0">+</span> indicator <span class="sy0">+</span> <span class="st0">'" style="vertical-align: baseline;" border="0" alt="In progress..." /></span>'</span><span class="br0">)</span><span class="sy0">;</span>
	<span class="br0">}</span>

</div></div> <p>change to </p> <div dir="ltr" class="mw-geshi mw-content-ltr"><div class="javascript source-javascript">

	<span class="kw1">var</span> indicator <span class="sy0">=</span> stylepath <span class="sy0">+</span> <span class="st0">'/common/progress-wheel.gif'</span><span class="sy0">,</span>
	url <span class="sy0">=</span> <span class="st0">'/api.php?format=json&action=query&prop=duplicatefiles&dflimit=500&generator=allimages&gailimit=500'</span><span class="sy0">;</span>
	dil <span class="sy0">=</span> dil <span class="sy0">||</span> <span class="br0">{</span><span class="br0">}</span><span class="sy0">;</span>
 
	<span class="kw1">if</span> <span class="br0">(</span><span class="sy0">!</span><span class="br0">(</span>$<span class="br0">(</span><span class="st0">'#dupImagesProgress'</span><span class="br0">)</span>.<span class="me1">length</span><span class="br0">)</span><span class="br0">)</span> <span class="br0">{</span>
		$<span class="br0">(</span><span class="st0">'#mw-dupimages'</span><span class="br0">)</span>.<span class="me1">prepend</span><span class="br0">(</span><span class="st0">'<div id="dupImagesProgress" style="height: 0; text-align: center;"><img src="'</span> <span class="sy0">+</span> indicator <span class="sy0">+</span> <span class="st0">'" style="border: 0 none;" alt="In progress..." /></div>'</span><span class="br0">)</span><span class="sy0">;</span>
	<span class="br0">}</span>

</div></div> <p>
</p>

Remove from the middle of the code

			$<span class="br0">(</span><span class="st0">'#dupImagesProgress'</span><span class="br0">)</span>.<span class="me1">hide</span><span class="br0">(</span><span class="br0">)</span><span class="sy0">;</span>

</div></div> <p>
</p>

Edit

			<span class="kw1">else</span> <span class="br0">{</span>
				<span class="kw1">if</span> <span class="br0">(</span>output<span class="br0">)</span> <span class="br0">{</span>
					$<span class="br0">(</span><span class="st0">'#mw-dupimages'</span><span class="br0">)</span>.<span class="me1">append</span><span class="br0">(</span>output <span class="sy0">+</span> <span class="st0">'</ul>'</span><span class="br0">)</span><span class="sy0">;</span>
				<span class="br0">}</span>
			<span class="br0">}</span>

</div></div> <p>change to </p> <div dir="ltr" class="mw-geshi mw-content-ltr"><div class="javascript source-javascript">

			<span class="kw1">else</span> <span class="br0">{</span>
				$<span class="br0">(</span><span class="st0">'#dupImagesProgress'</span><span class="br0">)</span>.<span class="me1">empty</span><span class="br0">(</span><span class="br0">)</span><span class="sy0">;</span>
				<span class="kw1">if</span> <span class="br0">(</span>output<span class="br0">)</span> <span class="br0">{</span>
					$<span class="br0">(</span><span class="st0">'#mw-dupimages'</span><span class="br0">)</span>.<span class="me1">append</span><span class="br0">(</span>output <span class="sy0">+</span> <span class="st0">'</ul>'</span><span class="br0">)</span><span class="sy0">;</span>
				<span class="br0">}</span>
			<span class="br0">}</span>

</div></div>

(Edited by Saftzie)

Dessamator·10/25/2015

<p>Oh okay, that should work too. </p>

(Edited by Dessamator)

Saftzie·10/28/2015

<p>A slightly better idea, I think, is to move the spinner to the middle of the top line, instead of the left, and set its layout height to 0. </p> <div dir="ltr" class="mw-geshi mw-content-ltr"><div class="javascript source-javascript">

		$<span class="br0">(</span><span class="st0">'#mw-dupimages'</span><span class="br0">)</span>.<span class="me1">prepend</span><span class="br0">(</span><span class="st0">'<div id="dupImagesProgress" style="height: 0; text-align: center;"><img src="'</span> <span class="sy0">+</span> indicator <span class="sy0">+</span> <span class="st0">'" style="border: 0 none;" alt="In progress..." /></div>'</span><span class="br0">)</span><span class="sy0">;</span>

</div></div> <p>
I'll make changes to the opening post and my sandbox incorporating the above edits. </p>

(Edited by Saftzie)

Cqm·11/19/2015

<p>I can't see where the current code is right now, is there a link I'm missing? As far as the code style goes, I'd be inclined to use the mediawiki.api module for the API requests as it's more future proof than working with the URL strings directly (in case someone wants to port it to wikipedia, etc.). It also saves having to remember to explicitly set the format parameter (which I'm always forgetting). Another MediaWiki specific idea is to use mw.config over accessing the global directly. I can never tell if Wikia rely on these being globals due to the complexity of how they handle globals in general, but it's probably a good idea. </p><p>As far as performance goes, obviously more requests are going to have side effects, but as requests are typically run async in JS is this that much of a performance hit? Numbers don't mean an awful lot without timing as well. </p>

(Edited by Cqm)

Saftzie·11/20/2015

<p>I incorporated the changes into the live version a few weeks ago. I intended for this review just to fix bugs, because the previous version was very buggy. The previous version is the link in the original post. </p><p>I'd argue the updated code performs better, since it returns more complete result sets. However, it can take more time (and possibly requests) to run on wikis that have a lot of duplicates, since the previous version didn't return everything. Not returning everything can be accomplished fastest by not running at all, eh? </p>

(Edited by Saftzie)

What do you think?