[RkWard-devel] New Plug-ins

[Thomas Friedrichsmeier]

Hi!

On Sunday 31 December 2006 01:16, SJR wrote:
> @ Thomas please give me some feed back about my changes and plug-ins
> regarding quality and so on.

Still haven't looked at everything in detail, but here are a few more 
comments:
1) Try to make sure the generated code is always left aligned (unless of 
course the indentation is meaningful). Right now, at many places the code is 
indented with one tab, even though it is top-level statements. Note that it 
does not matter much, whether or not you indent the PHP statements by one 
tab, as is currently done in some plugins, but not in others. However, 
everything between ?> and <? is directly visible to the user, and should be 
left-aligned.

2) When you find yourself using a radio widget with exactly two options, it's 
almost always better to use a checkbox instead. E.g.:

<radio id="narm_skewness" label="Calculate Skewness with Missing Values">
	<option value="na.rm = FALSE" label="FALSE"/>
	<option value="na.rm = TRUE" label="TRUE"/>
</radio>

(not that using that wording, you'd also have to exchange TRUE and FALSE) 
would become:

<checkbox id="narm_skewnwss" label="Exclude missing values" value="na.rm=TRUE" 
value_unchecked="na.rm=FALSE"/>

This uses less space, is easier to read, and less complicated all at the same 
time.

3) At some places you use something like

<frame label="The number of the selcted values must be between 3 and 
5000">...</frame>

to add a comment. I think it would be better not to use a frame, but rather 
add a <text>-element with that note, i.e.:

<text>
	Note: The selected vectors must be between 3 and 5000 in length.
</text>

Using a frame with a label is more suited for giving a caption, than for a 
comment. Also <text> will auto-break, and hence typically look better for 
everything longer than 20-30 characters

4) (Applies to almost all plugins, but I just noted it again while reviewing 
yours) We already have a sort of informal convention to place most temporary 
data in objects called rk.temp*. Probably we should try to actually stick to 
that convention at all places, and make sure we never write to any other 
symbols from plugins (unless the user explicitely asked for that, of course). 
Most importantly, we still have many cases where iterators (such as "var" 
or "i") from for statement are created in the workplace. Worse, if the user 
happens to already have own objects called "var" or "i" in the workspace, 
those are overwritten.
While it does make for a lot more writing, we should probably rename all those 
to rk.temp.something, too, and e.g. write

rk.temp.i=0;
for (rk.temp.var in rk.temp.vars) {
	rk.temp.i = rk.temp.i+1
	...
}

everywhere.
A small compensation for the added typing is that we can simplify the cleanup 
() code in most cases to

rm (list=grep ("^rk.temp", ls (), value=TRUE))

I'm willing to go through all existing plugins and fix this there (not today, 
though), but please use this for any further plugins, you create.

5) Thanks for creating all these new plugins! I just have a hard time keeping 
up with your pace ATM, so responses to all your new plugins and suggestions 
may come in slowly...

Regards
Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/rkward-devel/attachments/20070102/34b8648a/attachment.sig>