By using this site, you agree to our updated Privacy Policy and our Terms of Use. Manage your Cookies Settings.
455,847 Members | 1,375 Online
Bytes IT Community
+ Ask a Question
Need help? Post your question and get tips & solutions from a community of 455,847 IT Pros & Developers. It's quick & easy.

Efficient Code or Not?

eWish
Expert 100+
P: 971
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
Dec 31 '07 #1
Share this Question
Share on Google+
10 Replies


KevinADC
Expert 2.5K+
P: 4,059
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.
Dec 31 '07 #2

eWish
Expert 100+
P: 971
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
Dec 31 '07 #3

Kelicula
Expert 100+
P: 176
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.  
Jan 1 '08 #4

eWish
Expert 100+
P: 971
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
Jan 1 '08 #5

Kelicula
Expert 100+
P: 176
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.
Jan 1 '08 #6

KevinADC
Expert 2.5K+
P: 4,059
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.
Jan 1 '08 #7

eWish
Expert 100+
P: 971
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
Jan 2 '08 #8

KevinADC
Expert 2.5K+
P: 4,059
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";
Jan 2 '08 #9

Kelicula
Expert 100+
P: 176
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....
Jan 2 '08 #10

Kelicula
Expert 100+
P: 176
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
Feb 5 '08 #11

Post your reply

Sign in to post your reply or Sign up for a free account.