improve code - combine for & join

[Mike Solomon]

I have written the following code that does what I require but I am
sure it could be done better

How can I improve this

my @required = (
{company => "Company is required"},
{address1 => "Address1 is required"},
{postcode => "Postcode is required"},
{phone => "Phone is required"},
);

my (@key, @value);

for (@required) {
for (keys %{$_}) { push @key, qq {'$_'};}
for (values %{$_}) { push @value, qq {'$_'};}
}

my $key = join "," , @key;
my $value = join "," , @value;

return $key , $value;

 

[Brian McCauley]

I have written the following code that does what I require but I am
sure it could be done better

How can I improve this

my @required = (
{company => "Company is required"},
{address1 => "Address1 is required"},
{postcode => "Postcode is required"},
{phone => "Phone is required"},
);

I shall assume that the above is just example input data and is not
part of the 'this' you are seeking to improve. Obviously an array of
single element hashes is a very weird data structure and whatever the
data you are seeking to represent there's almost certainly a better
structure. But without knowing the problem I couldn't say what the
better structure is.

my (@key, @value);

for (@required) {
for (keys %{$_}) { push @key, qq {'$_'};}
for (values %{$_}) { push @value, qq {'$_'};}
}

my $key = join "," , @key;
my $value = join "," , @value;

return $key , $value;

The above is more simply written:

return join("," => map { "'$_'" } map { keys %$_ } @required),
join("," => map { "'$_'" } map { values %$_ } @required);

Or if you prefer:

return join(",", map "'$_'", map keys %$_, @required),
join(",", map "'$_'", map values %$_, @required);

--
\\ ( )
. _\\__[oo
.__/ \\ /\@
. l___\\
# ll l\\
###LL LL\\

 

[Gunnar Hjalmarsson]

I have written the following code that does what I require but I am
sure it could be done better

How can I improve this

It's obviously a subroutine. To be able to comment on it, I'd like to
know the context in which that subroutine is called. What are you
doing with those comma delimited strings that are returned?

 

[Mike Solomon]

I shall assume that the above is just example input data and is not
part of the 'this' you are seeking to improve. Obviously an array of
single element hashes is a very weird data structure and whatever the
data you are seeking to represent there's almost certainly a better
structure. But without knowing the problem I couldn't say what the
better structure is.


The above is more simply written:

return join("," => map { "'$_'" } map { keys %$_ } @required),
join("," => map { "'$_'" } map { values %$_ } @required);

Or if you prefer:

return join(",", map "'$_'", map keys %$_, @required),
join(",", map "'$_'", map values %$_, @required);

Sorry I should have said what the purpose of this code is for.

I am using it in various cgi programs that create some javascript

The data is a field name and a message

The order is important which is why I used an array

I could just have created the 2 strings to return but as I tend too
use this method in most of my cgi scripts I find the above data
structure easier to view and edit, also in some scripts I have a lot
of fields

I may well be missing an obvious simple way of doing this and any
further suggestions for improvement would be welcome

I like the map option and will probably use it

Thanks

 

[Brian McCauley]

I am using it in various cgi programs that create some javascript

The data is a field name and a message

The order is important which is why I used an array

But why did you use single element hashes?

The natural representation of an ordered list of pairs would be:

my @required = (
[company => "Company is required"],
[address1 => "Address1 is required"],
[postcode => "Postcode is required"],
[phone => "Phone is required"],
);

Or if you want more hash-like behavour and don't mind using
non-builtin types:

use Tie::IxHash;
tie my %required, 'Tie::IxHash' => (
company => "Company is required",
address1 => "Address1 is required",
postcode => "Postcode is required",
phone => "Phone is required",
);

--
\\ ( )
. _\\__[oo
.__/ \\ /\@
. l___\\
# ll l\\
###LL LL\\