Re: [Scaffoldhunter-devel] Fwd: Re: Random Subset Patch File version 2

[Till S. <til...@tu...>]

Hi Andrew, 
thank a lot for submitting the patch! As you may have noticed, nils already committed it in the trunk.  I have looked through the code / usability and have two further suggestions:

1. Performance: 
The current implementation has a quadratic runtime behavior. The single operations are fast, but it can take some seconds for a larger dataset. This is caused by the removal from the ArrayList which takes O(n) / linear time in the worst and average case. You can avoid this overhead by swapping the element which should be removed with the last element in the list and remove the last element afterwards (since you do not depend on the ordering of the list). For this purpose i already implemented a SwappingArrayList and i just committed it to the trunk (see the util package). That means you can just call the swapAndRemove method. 

2. The user interface: 
In my opinion we should not use two dialogs that pop up after each other. Two dialogs do fragment the workflow.  Furthermore, i can not read the complete title of the window in my window manager as there is not enough space. Therefore, the purpose of the text field has to be guessed (there are also window managers out there with no title at all ;) ).
-> We should create a label in front of text field that explains the dialog (something like: "please enter the size of the random subset"). I know the already implemented rename dialog was not the best example here ;) 
-> we should remove the second dialog and integrate the naming directly in the first dialog OR just create the subset with a fixed name (the user can rename it afterwards if the name does not fit. together with the below suggestion about the default naming, this seem a solutions that is not less usable than the first one / you can decide here)
-> the size field should only accept numbers (we do not need the explanation in the field anymore if we have a label)
-> we should give a more meaningful default name: e.g.: random(parent name, size). That means the user can directly see that this subset is created randomly from parent subset with a specific size).

This were more suggestions than i intended at the beginning of this mail ;-) Nevertheless, this are the usual things that come up after patch submission. 

Greetings
Till

Am Montag, 7. April 2014, 11:53:51 schrieb Nils Kriege:
> 
> ----------  Forwarded Message  ----------
> 
> Subject: Re: Random Subset Patch File version 2
> Date: Monday 07 April 2014, 10:40:02
> From: Nils Kriege <nil...@tu...>
> To: Anjenson <hot...@gm...>
> 
> Hi Andrew,
> 
> thank you for your work on this feature. Your code looks fine and I have 
> comitted it to the repository.
> 
> 
> Thanks!
>   Nils
> 
> On Friday 04 April 2014 20:02:05 Anjenson wrote:
> > Hi Nils,
> > 
> > Sending you fixed version.
> > 
> > Regards,
> > Andrew
> -----------------------------------------
-- 
Dipl.-Inf. Till Schäfer
TU Dortmund University
Chair 11 - Algorithm Engineering
Otto-Hahn-Str. 14 / Room 237
44227 Dortmund, Germany

e-mail: til...@cs...
phone: +49(231)755-7706
fax: +49(231)755-7740
web: http://ls11-www.cs.uni-dortmund.de/staff/schaefer
pgp: https://keyserver2.pgp.com/vkd/SubmitSearch.event?&&SearchCriteria=0xD84DED79