Hot answers tagged

30

I will go through your code line by line and give feedback. We will skip the general advice on don't use CGI as it's actually suited for what you are trying to do here. I wrote this answer in two stages, so some of the things here might have been said by other people. I'm attempting to provide full feedback, and I'm not copying from others. If several of us ...


22

This is some very nice-looking code, and it looks like it is working. My criticism falls into the following topics: Discussions about style, naming, tools used etc. I assume you have consciously settled on a certain style, but some aspects strike me as so unusual that I would like to talk about them. I too, like to overengineer. But there are some parts of ...


20

The fact that this was an interview question changes how I look at the code. The interviewer was almost certainly looking for one answer: "union-find data structure" or "disjoint sets data structure". You were being judged by whether you said those magic words within the first few seconds, could come up with something similar on your own, could come up ...


17

Consider starting the child process in a sandbox. /usr/bin/perl is likely safe from a malicious user since /usr/bin is typically locked down by the root user(s). But /home/demetri/bin/awklikeperl.pl could be replaced if the owner is not careful with permissions. Putting the process in a sandbox will not only protect the rest of your system from attack, ...


13

One suggestion, use use warnings; use strict; Will force you to be a better perl programmer, by enforcing more rigidity to your scripts, and have the intepreter catch any nasty issues that might be otherwise hard to debug.


13

No it is not safe: Classic off-by-one error. You reserve space for 3 additional entries with command[argc+3] but you add 4. argv[i+1] and command[i+4] will be out of bounds on the last iteration (last valid index is argv[argc-1] and command[argc+2] respectively) execv expects a NULL terminated array of NULL terminated strings. So the last entry in commands ...


13

There are a few things I see in here that I would suggest could be different. I don't like that the logic for determining whether a cell is a sink is on both Rainfall and Cell. In fact, both classes have the method called is_sink... My preference would be to move the logic on to the Cell, which already knows how to calculate it's neighbours.... and then ...


11

Use these. At first they will make things harder, because you'll get a few error messages, you'll need to declare all variables with my, etc. But very quickly they will make your life better. use strict; use warnings; Whenever you see repetitive code, consider writing a subroutine. sub get_user_input { my $message = shift; print $message, ': '; ...


11

First, always use strict; use warnings; at the top of your code. The strict pragma restricts you to a sensible subset of Perl, which removes many sources for errors. Also, it forces you to declare all your variables with my. The warnings are very helpful, but non-fatal errors. If you are on a fairly modern perl (10 or later), you can use the say function. ...


11

Well, this is actually pretty decent code. There still is a lot of stuff that could be improved, I tried to focus on some more relevant ones. Subroutine Prototypes In a declaration like sub foo ($@&*) { ... }, we call the weird thing in parens a prototype. The prototype primarily changes how a call to that sub is parsed, and can set properties like ...


11

Argument handling $defaultmail and $defaultip are poorly named — there is a naming inconsistency: $host ||= $defaultip; … $mailto ||= $defaultmail; I would also advise against introducing $defaultmail and $defaultip as variables at all. Either just write them in directly: my $host = (shift @ARGV) || '8.8.8.8'; # Google's nameserver my $...


11

In my testing, constructing one Perl script and running it repeatedly is much faster (0.5s versus 3.6s) then running a new Perl instance for each replacement: while read f; do echo "s/$f/$f/gi;" done < files.txt > s.pl while read fn; do perl -pi s.pl "$fn" echo "$fn | $count/$lines finished..." count=$((count+1)) done < filenames....


11

#!/usr/bin/perl -T use strict; use warnings; use CGI qw/ :all /; my $basepath = '/some/path/to'; my $f = param('f') || ''; # No interpolation, no double quotes. die if $f eq ''; # Don't use && for flow control. my $path = "$basepath/$f.pdf"; open my $PATH, '<', $path or die; # Lexical ...


10

tokenize The tokenize subroutine could be simplified: sub tokenize { my ($code) = @_; die "Invalid Arguments" unless defined $code; return $code =~ m!\d+|[-+*/.]!g; } Changes include: Shorter parameter name One-line validation Use global match in list context to produce a list of all matches Simpler regex that avoids leaning toothpick ...


10

Just one small point that @200_success probably left out so I can say something too: when matching something against $_, for example in if ($_ =~ /[0-9]+/) { ... }, you can simply omit the $_: if (/[0-9]+/) { # ... } It's "the default input and pattern-searching space". Read more about the $_ variable in man perlvar or on perldoc.perl.org. The main ...


10

use File::Slurp; File::Slurp is considered broken by a lot of people. A good alternative is Path::Tiny. It's also good practice to import only what you need when you use a module. That helps others to see where a function is coming from (like your read_file, which I tried to find in your code), and it helps to keep your namespace clean. use Path::Tiny '...


9

Your encryption scheme basically takes a string and does a one-to-one mapping of characters to 63 numbers (0 to 62). Ignoring this straightforward mapping of characters, your scheme basically is the i-th character of the ciphertext (1-indexed) is c[i] = (p[i] + H**i) % N, where N is the size of the alphabet (63), p[i] is i-th character of the plaintext, and ...


8

You can use -p option (see perl --help): assume loop like -n but print line also, like sed So the script becomes: echo hi | perl -pe 's/i/o/g' which is really compiled to: $ perl -MO=Deparse -pe 's/i/o/g' LINE: while (defined($_ = <ARGV>)) { s/i/o/g; } continue { die "-p destination: $!\n" unless print $_; } -e syntax OK


8

It's nice that you're using strict and warnings. Subroutine Arguments A subroutine should first unpack the argument list, e.g. like my ($foo, $bar) = @_;, then do some processing, and explicitly return a value. While we can return implicitly and can access items in @_ directly, both can be error-prone and should be avoided in most situations. For example, ...


8

This answer is going to talk about your style, and best practices. Some of it may be subjective. I'll go through the code line by line. use strict; use warnings; Good job! Modern Perl programs should always start with strict and warnings. If you want more detailed information on what's wrong, you can add use diagnostics, but remember to remove it before ...


8

I would use something like perl -lane '@h{@F[1..$#F]}=()if/^Host\b/;END{$,=" ";print keys %h}' -- file or perl -lane '@h{ @F[ 1 .. $#F ] } = () if /^Host\b/; END { $, = " "; print keys %h; }' -- file -l removes newlines from input and adds them to prints -n runs the code for each line of the input -...


7

Beyond what's been mentioned, I can suggest following a few formatting conventions to make your life easier in the long run: Keep lines to a reasonable length, inserting line breaks at logical points where necessary. This helps avoid having to scroll horizontally, as well as improving readability. 80 columns is generally accepted as the standard right ...


7

Here are some considerations when writing such functions: By passing the function all of the variables it needs (test1), you don't need to rely on global variables. This makes the function easier to test, more predictable (no side effects) and easier to refactor as you can move it to another module or file easily. If it relied on global variables, it would ...


7

You seem to have taken away little from my efforts to help you write good Perl code. In particular you must add use warnings to the top of every Perl program, which in this case would have resulted in line after line of errors like Scalar value @clauses[$s] better written as $clauses[$s] You also should use Perl's range iterator. The C-style for loop is ...


7

There are several modules on CPAN that are made for this task. They all reside in the Getopt namespace. Getopt::Long even comes with the standard distribution. So coming to your question: Yes, there are better ways. If you are interested in something more fancy, you could have a look at Getopt::Declare where you can write the usage page and fill your ...


7

It looks quite good. Perldoc, use strict, use warnings are all good practices. The formatting is good, and the code is readable. I don't recommend hard-coding the name of the data file in your program. That turns it from a general-purpose program into throwaway code. If you use the default filehandle <>, then Perl will do the "right thing": if a ...


7

I'm not a huge fan of reading all input in one one go and transform it in one and then write it in one go. It seems clumsy and will not scale properly (although in the given case it's probably not going to be a problem). The basic structure I'd go for is this: open my $ASM, '>', "$output.asm" or die "Cannot open file to write '$output.asm'\n:$!"; ...


7

Generally First, do not get put down by negative feedback like 'this code is a mess and it sucks'. Be proud of what you have developed and evolve. Save it, and look back at it in a few years. The most important thing about writing code, is solving problems at hand. The book "Perl best practices" might be a good place to start. Also you can use the module ...


7

I want some help in tuning this script if possible cause it takes time if I increased my array Yes, the script can be tuned. I will discuss optimizing you script at the end of this post. The warnings pragma also if I use warnings I get a lot of warnings which I'm unable to handle Yes correct, you will get many warnings in case you add the statement ...


7

I'm the lead engineer for the MongoDB Perl driver with a couple of thoughts for you: you're using 'authenticate', which is for a very old version of the driver which is not recommended for use. In the v1.x series, you should provide username/password in the URI or in MongoClient parameters. Most of the value of this module seems to be setting parameters ...


Only top voted, non community-wiki answers of a minimum length are eligible