|
Anonymous Monk has asked for the wisdom of the Perl Monks concerning the following question:
Hi,
Is there any shortest method to write the code. I would like to simplify this code.
#!/usr/bin/perl
$sur = [
{ 'one' => 'USA', 'two' => 'GOD', 'three' => '690
+557','four'=>'hello how are u',},
{ 'one' => 'USA', 'two' => 'GOD', 'three' => '690
+557','four'=>'I am fine',},
{ 'one' => 'UK', 'two' => 'GOD', 'three' => '6905
+58','four'=>'I am Okay',},
];
@array = @$sur;
%hash1=();
foreach $key(@array) {
%hash2 = %$key;
$one = $hash2{one};
$two = $hash2{two};
$three = $hash2{three};
$four = $hash2{four};
$firstrow = $one."<-->".$two."<-->".$four;
if($hash1{$three} ne "") {
$hash1{$three} = $hash1{$three}."####".$four;
}
else {
$hash1{$three} = $firstrow;
}
}
############## Output ########################
#while(($k,$v) = each(%hash1))
#{
#print "$k----->$v\n";
#}
#Result Hash contain
#'690557'=>'USA<=>GOD<=>hello how are u####I am fine'
#'690558'=>'UK<=>GOD<=>I am Okay'
############## End ###########################
|
Re: simplify the code
by Zaxo (Archbishop) on Oct 09, 2005 at 15:01 UTC
|
You don't need all those temporary variables in your loop,
for (@$sur) {
my $firstrow = join '<-->', @{$_}{qw/one two four/};
$hash1{$_->{'three'}} .=
$hash1{$_->{'three'}} ?
'####'.$_->{'four'} :
$firstrow;
}
Update: The whole loop in a single statement:
$hash1{$_->{'three'}} .= $hash1{$_->{'three'}} ?
'####' . $_->{'four'} :
join( '<-->', @{$_}{qw/one two four/})
for @$sur;
No temporaries at all.
| [reply]
[d/l]
[select] |
|
|
I wrote basically the same solution, but like having the loop variable (esp. since there's a use of $_ in the map.)
We both changed the conditional in different ways, from ne '' in the original to evaluates true in yours, and to existence in mine.
for my $hashref (@$sur) {
if (exists $hash1{$hashref->{three}}) {
$hash1{$hashref->{three}} .= "####$hashref->{four}";
}
else {
$hash1{$hashref->{three}} = join '<-->', map $hashref->{$_}, (qw(o
+ne two four));
}
}
| [reply]
[d/l] |
|
|
The second one looks quite intimidating, IMHO. The first one could be clearer if you chose your temporary variables just a tad better. The code I ended up writing looks is almost identical to your first loop, except for the choice of temporaries; and I think it looks a lot clearer.
Makeshifts last the longest.
| [reply] |
Re: simplify the code
by polettix (Vicar) on Oct 09, 2005 at 15:02 UTC
|
I suggest using strict</code> and <c>warnings, just to be on the safe side. Variable $sur doesn't seem to be necessary, you can write directly: my @array = ( { one => 'USA', ... }, {...}, ... );
I also wonder why you need hashes inside the array, and why you don't use (anonymous) arrays:my @array = ( [ 'USA', 'GOD', '690557', 'hello how are u' ], [ ... ],
+ ... );
which lets you say:foreach my $aref (@array) {
# If you really need them...
my ($one, $two, $three, $four) = @$aref;
# Etc. etc.
}
Flavio
perl -ple'$_=reverse' <<<ti.xittelop@oivalf
Don't fool yourself.
| [reply]
[d/l]
[select] |
Re: simplify the code
by Aristotle (Chancellor) on Oct 09, 2005 at 16:32 UTC
|
#!/usr/bin/perl
use strict;
use warnings;
use Data::Dumper;
my @sur = (
{ one => 'USA', two => 'GOD', three => 690557, four => 'hello how
+are u', },
{ one => 'USA', two => 'GOD', three => 690557, four => 'I am fine'
+, },
{ one => 'UK', two => 'GOD', three => 690558, four => 'I am Okay'
+, },
);
my %h;
foreach my $row ( @sur ){
my $three = $row->{ three };
$h{ $three } .= exists $h{ $three }
? "####" . $row->{ four }
: join "<-->", @{ $row }{ qw/one two four/ };
}
print Dumper \%h;
As mentioned by others your data structure seems a bit strange, but maybe it’s just example code.
Makeshifts last the longest. | [reply]
[d/l] |
Re: simplify the code
by davidrw (Prior) on Oct 09, 2005 at 15:09 UTC
|
first comment is that you should use strict; and use warnings; ..next, i think this shortens it up:
#!/usr/bin/perl
use strict;
use warnings;
use Data::Dumper;
my $sur = [
{ 'one' => 'USA', 'two' => 'GOD', 'three' => '690
+557','four'=>'hello how are u',},
{ 'one' => 'USA', 'two' => 'GOD', 'three' => '690
+557','four'=>'I am fine',},
{ 'one' => 'UK', 'two' => 'GOD', 'three' => '6905
+58','four'=>'I am Okay',},
];
my %h;
foreach ( @$sur ){
if( exists $h{ $_->{three} } ){
$h{ $_->{three} } .= "####" . $_->{four};
}else{
$h{ $_->{three} } = join "<-->", @{$_}{ qw/one two four/ };
}
}
print Dumper \%h;
I have to wonder though why @$sur is a hash with those keys instead of just an array ... can you explain where that data structure is coming from/what it's for? | [reply]
[d/l]
[select] |
Re: simplify the code
by ides (Deacon) on Oct 09, 2005 at 14:58 UTC
|
#!/usr/bin/perl
use strict;
my $sur = [
{ 'one' => 'USA', 'two' => 'GOD', 'three' => '690557', 'four' => 'h
+ello how are u', },
{ 'one' => 'USA', 'two' => 'GOD', 'three' => '690557', 'four' => 'I
+ am fine', },
{ 'one' => 'UK', 'two' => 'GOD', 'three' => '69058', 'four' => 'I
+am Okay', },
];
my %hash1;
foreach my $key ( @{$sur} ) {
my $firstrow = $$key{one} . '<-->' . $$key{two} .
'<-->' . $$key{four};
if( $hash1{ $$key{three} } ne '' ) {
$hash1{ $$key{three} } = $hash1{ $$key{three} } .
'####' . $key{four};
}
else {
$hash1{ $$key{three} } = $firstrow;
}
}
| [reply]
[d/l] |
Re: simplify the code
by ysth (Canon) on Oct 09, 2005 at 20:34 UTC
|
It seems to me that your initial datastructure isn't what you want; is it possible you could change it when you are collecting the data initially to something like:
{
'690557' =>
[
{ 'one' => 'USA', 'two' => 'GOD', 'four'=>'hello how are u' }
+,
{ 'one' => 'USA', 'two' => 'GOD', 'four'=>'I am fine' }
],
'690558' =>
[
{ 'one' => 'UK', 'two' => 'GOD', 'four'=>'I am Okay' }
]
}
or maybe:
{
'690557' =>
{
'one' => 'USA', 'two' => 'GOD',
'four'=> [ 'hello how are u', 'I am fine' ]
},
'690558' =>
{
'one' => 'UK', 'two' => 'GOD',
'four'=> [ 'I am Okay' ]
}
}
Don't copy a from a reference to an array or hash variable just to get things out of it;
just do for ... ( @$arrayref ) and $hashref->{$key}; see
references quick reference for guidance. | [reply]
[d/l]
[select] |
|
|
