Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [ice-dev] Handling loadInput in newly created items for tutorial

​Thanks very much Andrew!


Jay


Jay Jay Billings
Oak Ridge National Laboratory
Twitter Handle: @jayjaybillings
From: ice-dev-bounces@xxxxxxxxxxx <ice-dev-bounces@xxxxxxxxxxx> on behalf of Andrew Bennett <bennett.andr@xxxxxxxxx>
Sent: Wednesday, July 13, 2016 4:54 PM
To: ice developer discussions
Subject: Re: [ice-dev] Handling loadInput in newly created items for tutorial
 
Thanks Jay.  The part about knowing how and why when the error occurs makes sense.  I was under the impression that the goal of the alternative was to prevent the error altogether.  The whole issue seems to have arisen because when I started working on the original VIBE items I used the (incorrect) Nek500 and Proteus items as inspiration, and have carried that baggage with me. 

As for code cleanliness I agree with all you wrote.  At the time I believed that the if...else... qualified as a very specific low level use case (if no file, play it safe and get one), but now that I understand the risks of loadInput it's clear that they are different cases.

I'll make the changes to all of the items that have this issue and give them all proper loadDefault type operations.

Andrew

On Wed, Jul 13, 2016 at 12:58 PM, Billings, Jay Jay <billingsjj@xxxxxxxx> wrote:

Andrew,


Yes, it could still occur, but the point is that loadInput is called by other places in ICE and not intended to be called the way you were doing it. Specifically, loadInput should never be called from setupForm because it called after construction when the user specifies a file. There are situations where I could overwrite your Form with junk. For example, in the case with the Proteus and Nek5000 models, some services were not allocated yet and it barfed. 


So, the errors could still happen, but we will know exactly where and why.


The final issue here is one of code cleanliness. In general, when you are designing something, put things that perform different tasks into different functions until you have three or more examples that you can use to generalize (via induction) a single function. That does not mean that you take the code from each one and put it into a switch like "if (f != null) ... else ..." and call one function, but that you write one function with minimal branching that does the work of all three originals. (See our developer guide for why I hate switches.) In this case, that means we have loadDefaults() and loadInput(), which we will merge and chain in the future when we change the backend persistence model and construction scheme for the Items.


Jay


Jay Jay Billings
Oak Ridge National Laboratory
Twitter Handle: @jayjaybillings
From: ice-dev-bounces@xxxxxxxxxxx <ice-dev-bounces@xxxxxxxxxxx> on behalf of Andrew Bennett <bennett.andr@xxxxxxxxx>
Sent: Wednesday, July 13, 2016 2:19 PM
To: ice developer discussions
Subject: Re: [ice-dev] Handling loadInput in newly created items for tutorial
 
Sure we can do that.  I'm a little confused about how that alleviates the possibility of bugs though.  In the issue you wrote:

> The problem with setupForm() being part of the constructor means that the loadInput operation could be, but might not always be, called before the form is initialized properly or even before the project is properly configured.

Here's what confuses me.  In the old code setupForm did (essentially) this:

    form = new Form();
    ioService = getIOService();
    loadInput(null);

And the new code is going to be:

    form = new Form();
    ioService = getIOService();
    loadDefaults();

Where loadDefaults is going to just be the code from loadInput in the if(fileName == null) block.  Couldn't the situation outlined in your comment still occur with the new code?

Andrew



On Wed, Jul 13, 2016 at 11:00 AM, Billings, Jay Jay <billingsjj@xxxxxxxx> wrote:

Andrew,

​

Just put your code to read the file in another operation called loadDefaults() and call it from setupForm() for now. Then your template should work fine. That should be sufficient, right?


Jay


Jay Jay Billings
Oak Ridge National Laboratory
Twitter Handle: @jayjaybillings
From: ice-dev-bounces@xxxxxxxxxxx <ice-dev-bounces@xxxxxxxxxxx> on behalf of Andrew Bennett <bennett.andr@xxxxxxxxx>
Sent: Wednesday, July 13, 2016 12:57 PM
To: ice developer discussions
Subject: [ice-dev] Handling loadInput in newly created items for tutorial
 
On GitHub Alex, Jay, and I have been discussing how we can correctly implement a way to load default data into a form (or rather, how it was being incorrectly handled previously).  While the incorrect way was convenient it could cause problems.  The correct version of the code, unfortunately, loses some functionality that was a key component of the tutorial during the item creation portion.  We need a new way to handle that ASAP. 

My thoughts are we can ammend the tutorial docs to have the users import a data file when creating their new item, thus calling the loadInput method in the correct way.  This is not particularly more complex than what was previously being done, but I wanted to see if anyone has other thoughts on the matter.

Andrew

_______________________________________________
ice-dev mailing list
ice-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/ice-dev


_______________________________________________
ice-dev mailing list
ice-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/ice-dev

Back to the top