Review ShortCutEffect and establish best practices recommendations for refactoring

[fedorov]

The code in question is here (this is the code developed by @gaoyi, and being integrated into SlicerPathology by @ebremer ):

https://github.com/SBU-BMI/SlicerPathology/blob/master/ShortCutEffect/ShortCutEffect.py

It does not look like it follows the Editor effect development pattern, which (I believe) causes troubles improving the usability of the effect (e.g., currently user needs to press a keyboard key to start processing, and it is not obvious to @ebremer how to replace this with a UI button).

@pieper it would be very helpful if you could review this code with @naucoin after June travels and give us your assessment what is your recommendation on refactoring this code to improve robustness, usability and maintainability.

[pieper]

A few thoughts:

First it looks to me like there's a lot of leftover code in that class that could be cleaned up. Probably the best is to take a fresh approach by looking at other effects and deciding how best to adapt the segmenter into the Editor framework (the previous implementation always seemed to be trying to shoehorn in the conventions from a stand-alone prototype and it never worked very smoothly).

Second it might be good to do this fresh implementation in the Segment Editor extension since we plan to move that into the core once it stabilizes. Since the Segment Editor is being actively developed in a funded project and includes enhancements to the Editor codebase it would be the logical place to work.

Refer to this thread for more info:

http://slicer-devel.65872.n3.nabble.com/editor-I-O-issue-td4036752.html

On Thu, Jun 2, 2016 at 3:31 PM, Andrey Fedorov notifications@github.com wrote:

The code in question is here (this is the code developed by @gaoyi https://github.com/gaoyi, and being integrated into SlicerPathology by @ebremer https://github.com/ebremer ):

https://github.com/SBU-BMI/SlicerPathology/blob/master/ShortCutEffect/ShortCutEffect.py

It does not look like it follows the Editor effect development pattern, which (I believe) causes troubles improving the usability of the effect (e.g., currently user needs to press a keyboard key to start processing, and it is not obvious to @ebremer https://github.com/ebremer how to replace this with a UI button).

@pieper https://github.com/pieper it would be very helpful if you could review this code with @naucoin https://github.com/naucoin after June travels and give us your assessment what is your recommendation on refactoring this code to improve robustness, usability and maintainability.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SBU-BMI/SlicerPathology/issues/45, or mute the thread https://github.com/notifications/unsubscribe/AAHsfe-nNlVIm3PcbLHiXz7MH3KHELAbks5qHy-igaJpZM4Is5mE .

[fedorov]

Thank you Steve, this is helpful! I also thought starting from fresh might be good in the long term. It's a pity @ebremer won't be at the project week in Heidelberg to work on this refactoring with the Segment Editor team!

[fedorov]

I don't think we can complete this by the end of the project (Sept 1), especially considering that Segmentation Editor is still not totally finalized. Postponing until another day!

[tdiprima]

I'll take a look.

[tdiprima]

@pieper quick question — I'm new to the project and I was wondering what you meant by "leftover code". I just found two functions that don't do anything; they just return. (runQTCGA_Template() and runQTCGA_Refine_Curvature()) By any chance, is that what you meant?

Thanks.

First it looks to me like there's a lot of leftover code in that class that could be cleaned up.

[pieper]

@tdiprima I haven't looked in a while, but yes, I think some code that is basically stubbed out should probably be cleaned up. Also there are big blocks of commented out code.

Good to see this code being worked on again. Now that the segment editor has matured it may be even easier to integrate.

[fedorov]

@tdiprima before you start refining the code, it is a good idea to make sure the testing you have in place is sufficient to detect regressions that are likely to happen in the process of refactoring.

[tdiprima]

Andrey, yes. Thank you again. You're right. Erich and I were just talking about that. Steve, ok good... thank you for responding :)