Connecting Tech Pros Worldwide Forums | Help | Site Map
bytes > topic > perl > answers

Efficient Code or Not?

eWish's Avatar
Moderator
  
Join Date: Jul 2007
Location: Arkansas
Posts: 900
#1: Dec 31 '07
Will this style of coding cause me some speeding penalties? I am just trying to reduce the number of times I have to write an open function. Naturally, this is just an example.

Expand|Select|Wrap|Line Numbers
  1. my ($file_to_modify) = &set_log_file(3);
  2. my (@act_valid_log) = &read_data_file(3);
  3.  
  4. #do something with the result of @act_valid_log 
  5.  
  6.  
  7. sub set_log_file {
  8.  
  9.     my ($get_path) = @_;
  10.  
  11.     my ($return_path)     = $get_path == 1 ? qq{giftcard_log.log}
  12.                         : $get_path    == 2 ? qq{giftcard_db.log}
  13.                         : $get_path    == 3 ? qq{giftcard_act_valid.log}
  14.                         :                   &errorcode(__FILE__, __LINE__, "Your entry does not match anything.", "$!");    
  15.                         ;
  16.  
  17.     return ($return_path);
  18.  
  19. }
  20.  
  21. sub read_data_file {
  22.  
  23.     my ($file_to_read) = @_;
  24.     my ($giftcard_file) = &set_log_file($file_to_read);
  25.     my (@data);
  26.  
  27.         open(my $READ_DATA_FILE, '<', $giftcard_file) || &errorcode(__FILE__, __LINE__, $giftcard_file, "$!");    
  28.         @data = <$READ_DATA_FILE>;
  29.         close($READ_DATA_FILE);
  30.  
  31.     return(@data);
  32.  
  33. }
Looking for some feedback.

--Kevin

KevinADC's Avatar
Expert
  
Join Date: Jan 2007
Location: Southern California USA
Posts: 4,091
#2: Dec 31 '07

re: Efficient Code or Not?

Quote:

Originally Posted by eWish

Will this style of coding cause me some speeding penalties? I am just trying to reduce the number of times I have to write an open function. Naturally, this is just an example.

Expand|Select|Wrap|Line Numbers
  1. my ($file_to_modify) = &set_log_file(3);
  2. my (@act_valid_log) = &read_data_file(3);
  3.  
  4. #do something with the result of @act_valid_log 
  5.  
  6.  
  7. sub set_log_file {
  8.  
  9.     my ($get_path) = @_;
  10.  
  11.     my ($return_path)     = $get_path == 1 ? qq{giftcard_log.log}
  12.                         : $get_path    == 2 ? qq{giftcard_db.log}
  13.                         : $get_path    == 3 ? qq{giftcard_act_valid.log}
  14.                         :                   &errorcode(__FILE__, __LINE__, "Your entry does not match anything.", "$!");    
  15.                         ;
  16.  
  17.     return ($return_path);
  18.  
  19. }
  20.  
  21. sub read_data_file {
  22.  
  23.     my ($file_to_read) = @_;
  24.     my ($giftcard_file) = &set_log_file($file_to_read);
  25.     my (@data);
  26.  
  27.         open(my $READ_DATA_FILE, '<', $giftcard_file) || &errorcode(__FILE__, __LINE__, $giftcard_file, "$!");    
  28.         @data = <$READ_DATA_FILE>;
  29.         close($READ_DATA_FILE);
  30.  
  31.     return(@data);
  32.  
  33. }
Looking for some feedback.

--Kevin

It will be slightly more efficient if you just use one subroutine. set_log_file looks like it could be part of read_data_file instead of a seperate sub. But if you have other subs that call set_log_file that would be OK. Overall, there is nothing really inefficient about the code that I see. Personally I don't like to write a single scalar as a list assignment:

my ($file_to_read) = @_;

I would write that as:

my $file_to_read = $_[0];

although I am not sure if it is anymore efficient one way or the other, it's really a matter of personal preference I guess.
eWish's Avatar
Moderator
  
Join Date: Jul 2007
Location: Arkansas
Posts: 900
#3: Dec 31 '07

re: Efficient Code or Not?

Kevin Thanks for your comments and suggestions. I will take your advice on the $_[0] over @_ for a single value. That is a good idea.

--Kevin
Kelicula's Avatar
Expert
  
Join Date: Jul 2007
Posts: 169
#4: Jan 1 '08

re: Efficient Code or Not?

Quote:

Originally Posted by eWish

Kevin Thanks for your comments and suggestions. I will take your advice on the $_[0] over @_ for a single value. That is a good idea.

--Kevin


Also if you are always going to be sending only one parameter to the sub, you can use "shift".

Expand|Select|Wrap|Line Numbers
  1. #!/usr/bin/perl
  2.  
  3. my $var = &someSub(4);
  4.  
  5. sub someSub {
  6.  
  7. my $temp = shift; #grabs the first, (and only) value in the @_ array "4".
  8.  
  9. ...do something...
  10.  
  11. return something;
  12. }
  13.  
eWish's Avatar
Moderator
  
Join Date: Jul 2007
Location: Arkansas
Posts: 900
#5: Jan 1 '08

re: Efficient Code or Not?

Kelicula,

I am trying to keep the coding style as close to the existing code as I can. Currently it does not use the shift style of code.

--Kevin
Kelicula's Avatar
Expert
  
Join Date: Jul 2007
Posts: 169
#6: Jan 1 '08

re: Efficient Code or Not?

Quote:

Originally Posted by eWish

Kelicula,

I am trying to keep the coding style as close to the existing code as I can. Currently it does not use the shift style of code.

--Kevin


Oh, I see.
Now I understand. I was wondering when I replied before, cause I know you've used shift before.

OK.
KevinADC's Avatar
Expert
  
Join Date: Jan 2007
Location: Southern California USA
Posts: 4,091
#7: Jan 1 '08

re: Efficient Code or Not?

You seem to have a parenthesis fetish:

Expand|Select|Wrap|Line Numbers
  1. my ($file_to_read) = @_;
  2. my ($giftcard_file) = &set_log_file($file_to_read);
  3. my (@data);
none of the above lines needs the parenthesis on the left side, well the first one does but we already discussed that one. Is it just habit? It would be interesting to see if it affects efficiency or not. Bascially you are creating lists with one element. I wonder if perl has to take any extra steps to first make a list then make the assignment to the elements in the list. Maybe someone on perlmongers would know.
eWish's Avatar
Moderator
  
Join Date: Jul 2007
Location: Arkansas
Posts: 900
#8: Jan 2 '08

re: Efficient Code or Not?

I thought that a fetish with the P word parentheses was better that a fetish with the P word that ends in HP. :) Nonetheless, it is a habit and I have removed them.

Also, I decided to use a hash of named arguments. Which deviates form the original code. Then I used a hash for the log files in lieu of a subroutine.

--Kevin
KevinADC's Avatar
Expert
  
Join Date: Jan 2007
Location: Southern California USA
Posts: 4,091
#9: Jan 2 '08

re: Efficient Code or Not?

Quote:

Originally Posted by eWish

I thought that a fetish with the P word parentheses was better that a fetish with the P word that ends in HP. :) Nonetheless, it is a habit and I have removed them.

Also, I decided to use a hash of named arguments. Which deviates form the original code. Then I used a hash for the log files in lieu of a subroutine.

--Kevin

I don't know if the parentheses usage is really a concern or not. I see other perl scripts written that way and I think it is probably habit or just style preference. It could be much ado about nothing so if you prefer writing your code that way I would say continue to do so.

In some situations the parentheses are critical, but in the code you posted they are not. I just wonder if it causes any extra work for perl, like quoting scalars when they don't need quoting:

Expand|Select|Wrap|Line Numbers
  1. print "$foo";
Kelicula's Avatar
Expert
  
Join Date: Jul 2007
Posts: 169
#10: Jan 2 '08

re: Efficient Code or Not?

Try Benchmarking the script.

Expand|Select|Wrap|Line Numbers
  1.  
  2. #!/usr/bin/perl -w
  3.  
  4. use strict;
  5.  
  6. use Benchmark;
  7.  
  8. my $t = new Benchmark;
  9.  
  10. the code....
  11.  
  12. my $t2 = new Benchmark;
  13.  
  14. my $td = timediff($t2, $t);
  15.  
  16. print timestr($td);
  17.  
  18.  
See how long it takes to run, with then without p's.

Curious....
Kelicula's Avatar
Expert
  
Join Date: Jul 2007
Posts: 169
#11: Feb 5 '08

re: Efficient Code or Not?

I hear the switch operator is more efficient with numeric equalities.

eg:

Expand|Select|Wrap|Line Numbers
  1.  
  2. use Switch;
  3.  
  4. switch ($val) {
  5.     case 1        { print "number 1" }
  6.     case "a"    { print "string a" }
  7.     case [1..10,42]    { print "number in list" }
  8.     case (@array)    { print "number in list" }
  9.     case /\w+/    { print "pattern" }
  10.     case qr/\w+/    { print "pattern" }
  11.     case (%hash)    { print "entry in hash" }
  12.     case (\%hash)    { print "entry in hash" }
  13.     case (\&sub)    { print "arg to subroutine" }
  14.     else        { print "previous case not true" }
  15.     }
  16.  
See this... Switch
Reply

« previous question | next question »

Similar Perl bytes

/bytes/development

/bytes/it

Forums
Visit our community forums for general discussions and latest on Bytes

Our staff and community areas have moved to bytes.com/forums/

/bytes/about

We are a network of experts and professionals in IT and software development that help one another with answers to tough questions and share insights. Get the best answers to your questions from over 226,419 network members.

dev questions:
algorithms | asp | asp.net | c/c++ | coldfusion | c# | flash | html/css | java | javascript |
mobile dev | .net | perl | php | python | ruby | software dev | vb.net | visual basic | xml
it questions:
access | apache | careers | db2 | iis | macosx | mysql | networking | oracle | postgresql | sql server | unix | windows

About Bytes | Copyright 1995-2009 BYTES. All rights Reserved