Re: The maybe it is better written this way tool
by toolic (Bishop) on Nov 15, 2009 at 00:34 UTC
|
a few examples that I don't think Perl::Critic or any of its extensions covers
my $x;
foreach $x () {}
I uploaded your 2 lines of code to the free Perl::Critic website, http://perlcritic.com/, and it produces this message:
Loop iterator is not lexical at line 2, column 1. See page 108 of PBP
+. Severity: 5
It even has a link to the verbose explanation of the message, complete with advice on how to re-code it. Therefore, Perl::Critic does indeed detect this coding style.
I know Perl::Critic is configurable, and I would not be surprised to find out that it is also extensible. | [reply] [d/l] [select] |
|
| [reply] |
Re: The maybe it is better written this way tool
by ikegami (Patriarch) on Nov 14, 2009 at 22:03 UTC
|
@files = `ls *.xml|more`;
This example is pretty far fetched. You're asking for a tool to assist *Perl* programmers that understands (correctly or otherwise) that the *shell* command
ls *.xml | more
is a bad way of writing
echo *.xml
A tool that converts
chomp( my @files = `echo *.xml` );
to the better
my @files = glob('*.xml');
is already pretty far fetched.
Yet another example would be the extensive processing of values from @ARGV that could be replaced by the use of Getopt::Long.
Seems to me that "processing @ARGV" is hard to quantify into a test.
That said, there does exist a framework for notifying users of bad code. It's Perl::Critic. It doesn't modify the code since the idea is to identify as many possible problems as possible. You'd expect false positives here. Presumably a user willing to use Perl::Critic will look at the documentation to figure out what the warning he gets means. The recommended change can be placed there.
Now I notice you mentioned Perl::Critic. You seem to dismiss it. Why is that? Because no one coded all your suggestions yet? ( Apparently, it already checks for your first example )
| [reply] [d/l] [select] |
|
Err, I don't understand what you meant by far fetched? That it will be difficult to pro-grammatically understand the intention? Yes I guess it will. Did you mean something else?
"processing @ARGV" is hard to quantify into a test.
That's why I am asking here for suggestions but in what to check and if it seems to be possible. Regarding @ARGV I would look for 2 or more occurrences of either \$ARGV\[ or shift; outside of any sub or shift @ARGV; and then recommend.
Funny how easily we can misunderstand each other. I mentioned Perl::Critic in my post in order to acknowledge it and to show I am aware of it and not in order dismiss it. Not at all. Though I have not checked Perl::Critic for these I am looking for things that are probably not covered by Perl::Critic (yet?) as they are not really Perl coding styles. If the implementation will be another extension of Perl::Critic or something else is another matter. It would be probably easier for the user (and for the Padre integration) if it was built on top of Perl::Critic.
As it was pointed out by others at least one of the examples I gave is already covered by Perl::Critic. I should read the book again...
| [reply] |
|
I don't understand what you meant by far fetched?
Unless you were suggesting the tool should only look for that specific string, the tool would have to know
- That more's output is being piped.
- How more behaves when it's output is being piped.
- The directory from which ls is launched.
- That the file spec will only match the names of file (not directories).
- That ls is given a list of file names (not directories).
- How ls behaves when given a list of file names.
It would also have to make assumptions such as
- sh is used as the shell.
- ls and more aren't aliases.
- ls and more refer to the standard utilities.
All this from a program that's suppose to know Perl. And that's just to handle that one command.
That it will be difficult to pro-grammatically understand the intention? Yes I guess it will
That it requires an incredible amount of knowledge about non-Perl material to have the slightest clue as to the command's meaning.
Regarding @ARGV I would look for 2 or more occurrences of either \$ARGV\[ or shift; outside of any sub or shift @ARGV; and then recommend.
That's no good. Whether you're doing it "correctly" or not, all you have to do with @ARGV is loop over it. The number of times @ARGV is referenced (i.e. how you loop over it) does not indicate how its contents are used at all.
| [reply] [d/l] [select] |
|
|
Re: The maybe it is better written this way tool
by educated_foo (Vicar) on Nov 14, 2009 at 23:09 UTC
|
This same urge brought us clippy. I don't think anyone has figured out how to do this in a non-annoying way. | [reply] |
|
| [reply] |
|
| [reply] |
Re: The maybe it is better written this way tool
by Anonymous Monk on Nov 14, 2009 at 21:01 UTC
|
| [reply] [d/l] [select] |
Re: The maybe it is better written this way tool
by Anonymous Monk on Nov 14, 2009 at 21:08 UTC
|
$#array refers to an index value into the array @array. It should never be used to determine the number of elements of @array. For example:
if ( $#ARGV < 2 ) {
Should be written as:
if ( @ARGV < 3 ) {
| [reply] [d/l] [select] |
|
| [reply] [d/l] |
|
$[ = 10;
my @arr = ('a', 'b');
use Data::Dumper;
warn Dumper [ $#arr, scalar @arr ];
__END__
$VAR1 = [
'11',
2
];
Actually I think it would be better if this tool will suggest not to change $[ in the first place. | [reply] [d/l] [select] |
|
|
$array[ 5 ]
Isn't that DWIM?
Why does @array in scalar context evaluate to the number of elements? Why not just use $#array + 1 and have an array in scalar context return the first element, or the sum of all the elements, or the total length of all the elements?
| [reply] [d/l] [select] |
|
|
|
Re: The maybe it is better written this way tool
by Anonymous Monk on Nov 14, 2009 at 20:58 UTC
|
Change split / /, $var to split ' ', $var
| [reply] [d/l] [select] |
|
On the same note, split '...' is more readable as split /.../, since the first arg of split is a regex pattern. (split ' ' excepted, of course.) We often get questions about split '|'.
| [reply] [d/l] [select] |
|
> On the same note, split '...' is more readable as split /.../, since the first arg of split is a regex pattern. (split ' ' excepted, of course.) We often get questions about split '|'.
I don't understand, don't you mean it the other way round?
IMHO since /.../ clearly shows that it's a regex it should better always be written like that.
'...' misleadingly looks like a simple string.
| [reply] |
|
|