Sent from my iPhone
On Sep 7, 2015, at 2:16 PM, Nicolai Hess <nicolaihess@web.de> wrote:
*Now* I understand this rule.
Hi Nicolai, points to the need for some very good doc for these rules :-)
2015-09-02 3:40 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
On Tue, Sep 1, 2015 at 1:29 PM, Nicolai Hess <nicolaihess@web.de> wrote: Thank you all for your response, now the interesting question is, what should this rule recommend to use instead? First assign to a block local variable and add a nil check in the unwind block?
No. I think the pattern modifies this:
| t | ... [t := initExpr. exprs] ensure: [t msg]
to this
| t | ... t := initExpr. [exprs] ensure: [t msg]
Control never reaches the block [exprs] isf there is an error in initExpr so there's no issue with t being unassigned; control won't get to the ensure block, so no need to test for nil.
And maybe we should improve that rule, because in Pharo 5.0 there are 5 methods catched by this rule: 3 have an explicit ifnonil check before accessing the variable - their implementation actually behaves correctly. The other two are just "testdata" used in the unit test for this rule
2015-08-31 15:30 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
If there's an error in the expression that is assigned to file, and the stack unwound, file won't have been assigned and the close will get sent to nil. The assignment to file should precede the block.
Sent from my iPhone
On Aug 31, 2015, at 5:03 AM, Nicolai Hess <nicolaihess@web.de> wrote:
Anyone? We may remove this rule if no one understands for what this is good for. (if anyone knows, please comment that class).
2014-12-14 11:22 GMT+01:00 Nicolai Hess <nicolaihess@web.de>:
2014-11-30 20:54 GMT+01:00 stepharo <stepharo@free.fr>:
thanks we should open a bug entry to improve the class comment of this rule. Any taker?
Stef
Le 30/11/14 12:10, Thierry Goubier a écrit :
done 14618 Add a comment describing RBFileBlocksRule
now I just need to have a good description for this rule. Please, can someone outline the discussion on this thread: - describe why code matching this rule is considered back practice - describe how the code could be rewritten.
nicolai
Le 30/11/2014 12:01, Max Leske a écrit :
On 30.11.2014, at 11:18, Thierry Goubier <thierry.goubier@gmail.com> wrote:
Le 30/11/2014 11:06, Max Leske a écrit :
The code would look like this:
[ | file | file := StandardFileStream forceNewFileNamed: âfooâ. file nextPutAll: âbarâ ] ensure: [ file close ].
Why that is considered bad practice however, I canât tell.
This one would fail because file is a temp to the block (and not visible to the ensure block).
If it werenât visible, wouldnât the compiler raise an exception?
In your example, yes.
But the rule matched:
| file | [ file := StandardFileStream forceNewFileNamed: âfooâ. file nextPutAll: âbarâ ] ensure: [ file close ].
(Hum: wouldn't the compiler complain of file might be nil in the ensure block?)
Another problem is that, if you have a failure in forceNewFileNamed:, file is nil and the ensure: fail as well.
True, but thatâs a problem this rule doesnât really cover I guess. That oneâs up to the developer.
I would expect that rule to flag bad coding practices, and I think it does ;)
Thierry
-- _,,,^..^,,,_ best, Eliot