*Now* I understand this rule. 2015-09-02 3:40 GMT+02:00 Eliot Miranda <eliot.mira...@gmail.com>:
> > > On Tue, Sep 1, 2015 at 1:29 PM, Nicolai Hess <nicolaih...@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.mira...@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 <nicolaih...@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 <nicolaih...@web.de>: >>> >>>> >>>> 2014-11-30 20:54 GMT+01:00 stepharo <steph...@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 <https://pharo.fogbugz.com/default.asp?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.goub...@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 >
