CodeMentor - getting picky now

The CodeMentor has a "Possible bug" test for methods answering the
default value in a partial ifTrue/ifFalse test. So...

^a ifTrue: [b]

should, in an ideal world, be explicitly coded as

^a ifTrue: [b] ifFalse: [nil]

The CodeMentor doesn't have a test for the ifNil/ifNotNil construction,
perhaps it should?  It's easy to forget (as I just have) that the
seemingly equivalent

^a ifNil: [b]

doesn't answer nil if the test fails...

^a ifNil: [b] ifNotNil: [a]

--
Ian

Use the Reply-To address to contact me.
Mail sent to the From address is ignored.

And could the mentor also ignore unreferenced methods if they are methods
beginning with 'test' which are a kindOf: TestCase.

(I think there are a few things on people's picky lists...;-) Still the
mentor is a great tool...


"Ian Bartholomew" <[hidden email]> wrote in message
news:[hidden email]...

Tim,

> And could the mentor also ignore unreferenced methods if they are methods
> beginning with 'test' which are a kindOf: TestCase.

As I was playing around with the Lint rules, implementing the ifNil test
I wanted, I had a look at your wish list entry as well.  If you make the
following change then _in a new browser_ you shouldn't see the test cases.

BlockLintRule>>implementedNotSent
        | detector |
        detector := self new.
        detector name: 'Methods that implement messages which are never sent'.
        detector
                methodBlock:
                        [:context :result |
                        (context uses: context selector)

"here"
                                | ((context selector beginsWith: 'test') & (context selectedClass
includesBehavior: TestCase))
"to here"

                                ifFalse: [result addClass: context selectedClass selector: context
selector]].
        ^detector

--
Ian

Use the Reply-To address to contact me.
Mail sent to the From address is ignored.

Ian Bartholomew wrote:
> The CodeMentor has a "Possible bug" test for methods answering the
> default value in a partial ifTrue/ifFalse test. So...
>
> ^a ifTrue: [b]
>
> should, in an ideal world, be explicitly coded as
>
> ^a ifTrue: [b] ifFalse: [nil]

I added this check after finding some bugs where the ifFalse: branch was
omitted. Furthermore, many people do not know what #ifTrue: returns if
the receiver is false. Therefore, I prefer to make it explicit by adding
the "ifFalse: [nil]".

There are two main places where these still occur in my code. The first
is after I extract a method from a block. If the code is at the end of a
block, then it will add a return to the method when it might not need
it, so you can end up with an unnecessary return in front of the
#ifTrue:. These can be removed after I run the rule.

The other main place where it occurs is when I want to return from a
method and I don't care what is returned. I could have added a "^self"
after the #ifTrue:, but I didn't.

> The CodeMentor doesn't have a test for the ifNil/ifNotNil construction,
> perhaps it should?  It's easy to forget (as I just have) that the
> seemingly equivalent
>
> ^a ifNil: [b]
>
> doesn't answer nil if the test fails...
>
> ^a ifNil: [b] ifNotNil: [a]

I'm guessing that most people using the #ifNil: know that it returns the
receiver if it isn't nil. Whereas, most people don't know what #ifTrue:
returns when the receiver is false. It is similar to the #add: method.
Many people believe that it returns the receiver when it returns the
argument.

BTW, if you decide that a method shouldn't be displayed for a particular
rule, you can "Ignore this rule failure" in the results browser method list.

Andy/Blair, there is a bug in the "Ignore this rule failure", if you
select multiple methods and try to ignore the failures, you get an error
saying that "Exactly one object must be selected".


John Brant

> "here"
> | ((context selector beginsWith: 'test') & (context selectedClass
> includesBehavior: TestCase))
> "to here"

Awesome Ian... that gives me a headstart on learning Lint checking rules
too...

I think there are other rules that should be added - e.g. Method Too long
probably shouldn't apply to #createComponents etc.

Tim

Tim,

> I think there are other rules that should be added - e.g. Method Too long
> probably shouldn't apply to #createComponents etc.

It looks like a similar tweak to BlockLintRules class>>longMethod should
achieve that.

I wouldn't get too keen on changing the methods though.  I think it's OK
for a temporary tweak, I found all the "^test ifNil:" statements I was
looking for :-), but, like all base image modifications, making
permanent changes could prove a bit troublesome when the next Dolphin
version comes out.

John's post, in the other thread, describes a method of ignoring certain
  selectors and, personally, I think that would be the better way to go.

--
Ian

Use the Reply-To address to contact me.
Mail sent to the From address is ignored.

John,

Thanks for the replies.

> I'm guessing that most people using the #ifNil: know that it returns the
> receiver if it isn't nil. Whereas, most people don't know what #ifTrue:
> returns when the receiver is false.

You could well be right but as ifNil is a latish arrival in Dolphin it's
behaviour might not be as deeply ingrained here as ifTrue is.  I have to
admit that when I was tidying up some code this afternoon I made some
changes using ifNil that, um, didn't quite work as originally planned :-)

FWIW, I was going through trying to make my code more "intention
revealing", but I ended up going back through it and removing a lot of
the extra clauses.

> BTW, if you decide that a method shouldn't be displayed for a particular
> rule, you can "Ignore this rule failure" in the results browser method list.

Now I didn't know that, thanks for pointing it out.  I had tried right
clicking on the selector in the CodeMentor view, to see if I could
ignore individual methods, but I never thought of opening up the
MethodBrowser and doing it there.

Does this mean I should have read the manual <sigh>

--
Ian

Use the Reply-To address to contact me.
Mail sent to the From address is ignored.

"John Brant" <[hidden email]> wrote in message
news:l5Fkf.595734$x96.269772@attbi_s72...
>..
> Andy/Blair, there is a bug in the "Ignore this rule failure", if you
> select multiple methods and try to ignore the failures, you get an error
> saying that "Exactly one object must be selected".
>

Thanks John, #2046.

Regards

Blair