<br><br><div class="gmail_quote">On Wed, Jan 30, 2013 at 8:42 AM, Eygene Ryabinkin <span dir="ltr"><<a href="mailto:rea@codelabs.ru" target="_blank">rea@codelabs.ru</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">> I'm inclined to prefer the simple "if" variant anyway, since</div><div class="im">
> spreading config testing everwhere is, IMHO a bad design anyway and<br>
> hard to maintain.<br>
<br>
</div>Could you, please, elaborate on the "bad design" and "hard to maintain"?<br>
<div class="im"><br></div></blockquote><div>When you have such code (which is mine, and I'm not proud):</div><div><br></div><div>key = "singlethread" # Or worse, can come from the top class, whatever</div>
<div>config.get("Repository " + repository.getname(), key)</div><div><br></div><div>Let's say later on, we decide to rename the config option to "nothread"</div><div>Then if you grep the code, it's impossible to find such a pattern, so you'll likely introduce a bug.</div>
<div><br></div><div>Compared to:</div><div>config.repository[repository.getname()].singlethread</div><div><br></div><div>Then, you can grep the code and you'll find all the places where such "key" is used.</div>
<div>Better, when you need to document all the config option (a work I'm currently trying to do), you can grep for "config\." and get a list of all the config options directly, you don't have to run the software with breakpoint to figure out what was the value of 'key' at runtime.</div>
<div><br></div><div>I consider the later code easier to maintain, obviously, and the design, being a bit more rigid, would impose a better code, since you must know at write time what is the key you're reading, and if you do something wrong, it raises immediately (in the former case, you'd get empty value, which will likely break, *way later*).</div>
<div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> So the improved safety here might not worth it.<br>
<br>
</div>For me, any improved safety and general uniformity of code (remember,<br>
we have two places where we do single/multithreaded switch,<br>
folder/Base.py and accounts.py) outweights the rest in all, but rare<br>
cases of performance-critical path. So relying only at suggestthreads()<br>
is a premature optimization as Dr. Knuth 'sez ;))<br>
<div class="im"><br></div></blockquote><div>Agreed.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">> However, I still don't like config testing everywhere in the code as</div>
<div class="im">
> it's a pain to maintain and document.<br>
<br>
</div>Again, the explicit arguments for "pain to maintain and document" for<br>
this very case will be appreciated.<br></blockquote><div>The more test you have spread in the code, based on an config option, the more likely you'll miss one if you change the config.</div><div>Compare this:</div><div>
<br></div><div>class SingleThreadImap:</div><div> # do single thread operations</div><div><br></div><div>class MultiThreadImap: # inherit SingleThreadImap</div><div> # override the methods that can benefit from multithreading</div>
<div><br></div><div>In syncaccount function:</div><div> imapFolder = SingleThreadImap() if config.runtime.singlethread else MultiThreadImap()</div><div> imapFolder.syncmessage()</div><div><br></div><div>To that: </div>
<div>In offlineimap.py.run:</div><div> if config.get("general", "single-thread") == "True":</div><div> # run single threaded</div><div> else:</div><div> # start thread pool</div><div>
<br></div><div>In the syncaccount function</div><div> if imapFolder.suggestthreads() and not config.get("general", "singlethread") == "True":</div><div> # launch thread for syncmessage</div>
<div> else:</div><div> # launch syncmessage here</div><div><br></div><div>I think the former is easier:</div><div>1) it's more clear (you have a file called "SingleThreadImap" and another one called "MultiThreadImap", which is obvious when you're looking for a bug in either mode)</div>
<div>2) you only test the config once (re-read the second code above, and you'll see that I've introduced a bug that would be quite impossible to debug if I haven't told you)</div><div>3) You don't need to pass your config object around (again, easier to maintain if you need to change the config object)</div>
<div><br></div><div>What do you think?</div><div><br></div><div>Cyril</div></div>