New checks: UseHeadNotApply, UseLastNotApply, UseHeadOptionNotIf, UseZipWithIndexNotZipIndices

[vdichev]

New checks:

• l(0) can be replaced with a.head for collections. It might be controversial for arrays as index access is typical so I have not added it for these
• l(l.length -1) can be replaced with l.last
• if (l.nonEmpty) Some(l.head) else None can be replaced with l.headOption. I've added a couple of variants of this. I haven't added if (!l.nonEmpty) because it seems to me that double negation should be handled by a separate check.
• similarly, if (l.nonEmpty) Some(l.last) else None can be replaced with l.lastOption
• l.zip(l.indices) can be replaced with l.zipWithIndex

This pull request also interacts with a couple of other features so had to remove a couple noList checks.

[vdichev]

Sorry for sending so many new commits, I think this should be it now.

[HairyFotr]

After checking some projects, seq(0) and seq(seq.length-1) very often show up along with seq(1) or seq(seq.length-2), and in such cases I think seq(0) looks more readable than seq.head. What do you think about checking that other seq(...) calls do not occur in the same context (e.g. in the same method body, or class body if written there ...)?

:+1: :+1: :+1: for the other checks, they're definitely useful.

[vdichev]

Very good point, my thoughts were that seq(0) doesn't make sense when index access isn't recommended for any index, like for lists where it's linearly slow. How about if we restrict it to just List?

At any rate, these were the controversial ones in this pull request and I would be happy if you just pick the others and leave UseHeadNotApply/UseLastNotApply for now.

[HairyFotr]

Being strict for List could work, I'm only hoping it won't be too noisy, as people probably ignore the inefficiency of List for short lists.

I'll pick and merge the other checks tomorrow.

[vdichev]

For Lists, at least l(l.length - 1) seems pretty bad- even for small lists it reads much longer and traverses the list twice. For l(0) and l(1) together, I think it's idiomatic in functional languages to use l.head and l.head.head. Even then, wouldn't it be easy to avoid the warning if one just uses some of the traits like Seq?

In several Scala OSS projects- Play, Akka, Anorm, cats, dispatch, doobie, shapeless I don't see l(l.length - 1) except in Strings. At a first glance, there was just one usage of l(0) for a List, and that was in a test. All the others were in arrays, strings, vectors, seq.

Anyway, I don't feel as strongly about UseHeadNotApply than for UseLastNotApply. Let me know what you find in the codebase you're testing against!

[HairyFotr]

Cool, you've checked - sorry, I was mostly thinking out loud what might be a problem. Please add the commit with the limit to List, and I'll merge the whole thing.

It'd awesome to setup Linter into scala community build or something similar to make it easier to test well and compare - I usually just quickly run it on a few projects I have checked out.