Need help/advice to improve script

[sopan.shewale]

Hello,

I have Apache htpasswd format password store. The fields are ":"
separated and has more fields than standard format. I have script to
read the fields and load into Hash.

I need advice/help to improve the script. The current one works-but
not really good script. The script goes as follow:
--------------------------------
#!/usr/bin/perl
use strict;
use warnings;

my $data = {};

for (<DATA>) {

my $line = $_;
if ( defined $line ) {
if ( $line =~ /^(.*?).*?).*?).*?).*?)(?:.*))?$/
|| $line =~ /^(.*?).*?).*?).*?)(?:.*))?$/
|| $line =~ /^(.*?).*?).*?)(?:.*))?$/
|| $line =~ /^(.*?).*?)(?:.*))?$/ )
{
$data->{$1}->{pass} = $2;
$data->{$1}->{emails} = $3 || '';
$data->{$1}->{flag} =
( ( defined $4 ) && ( $4 == 0 ) ) ? 0 : ( $4 || '' );
$data->{$1}->{pass_change} = $5 || '';
$data->{$1}->{flag_change} = $6 || '';
}
}

}

use Data:umper;
print Data:umper->Dump( [$data] );

__DATA__
AllPresent:hjliEO35kCgwI:[email protected]:1:23232:24324
LastMissing:CyL92g3OKi.jM:[email protected]:1:2323
FlagZero:CyL92g3OKi.jM:[email protected]:0:23232
OnlyFlag:ZuqpLZ7AxHBvw[email protected]:0
RestMissing:ZuqpLZ7AxHBvw:[email protected]

-------------------------
Please note - only first two fields are must fields, rest may be
missing from lines. first is - username, second is encrypted
password, third is email, fourth is flag to force user to change
password(0-pass change is not required, 1-user must change pass),
fifth- the last time password changed epoch time, sixth- time of
change of flag (some times flag is changed by admins of the system).

thanks in advance

 

[Martijn Lievaart]

Hello,

I have Apache htpasswd format password store. The fields are ":"
separated and has more fields than standard format. I have script to
read the fields and load into Hash.

I need advice/help to improve the script. The current one works-but not
really good script. The script goes as follow:
--------------------------------
#!/usr/bin/perl
use strict;
use warnings;

my $data = {};

Simpler to use a hash variable, if needed you can always take a reference
to that.

for (<DATA>) {

my $line = $_;
if ( defined $line ) {
if ( $line =~ /^(.*?).*?).*?).*?).*?)(?:.*))?$/
|| $line =~ /^(.*?).*?).*?).*?)(?:.*))?$/ || $line
=~ /^(.*?).*?).*?)(?:.*))?$/ || $line =~
/^(.*?).*?)(?:.*))?$/ )

This is why split() was invented

{
$data->{$1}->{pass} = $2;
$data->{$1}->{emails} = $3 || '';
$data->{$1}->{flag} =
( ( defined $4 ) && ( $4 == 0 ) ) ? 0 : ( $4 || '' );
$data->{$1}->{pass_change} = $5 || '';
$data->{$1}->{flag_change} = $6 || '';
}
}

}

#!/usr/bin/perl
use strict;
use warnings;

my %data;

while (my $line = <DATA>) {
# chop of newline
chomp $line;
# split into fields
my ($user, $pass, $email, $flag, $passchange, $flagchange) =
split /:/, $line, 6;
# stuff into hash, using a hash slice
@{$data{$user}}{qw/pass email flag passchange flagchange/} =
($pass, $email, $flag, $passchange, $flagchange);
}

use Data:umper;
print Data:umper->Dump( [\%data] );

__DATA__
AllPresent:hjliEO35kCgwI:[email protected]:1:23232:24324
LastMissing:CyL92g3OKi.jM:[email protected]:1:2323
FlagZero:CyL92g3OKi.jM:[email protected]:0:23232
OnlyFlag:ZuqpLZ7AxHBvw[email protected]:0
RestMissing:ZuqpLZ7AxHBvw:[email protected]


The line with he hash slice can also be written as:
$data{$user}{pass} = $pass;
$data{$user}{email} = $email;
... etc ...

And obviously, it can even be shortened to:
# split and stuff into hash, using a hash slice
@{$data{$user}}{qw/pass email flag passchange flagchange/} =
split /:/, $line, 6;

HTH,
M4

 

[Jürgen Exner]

Hello,

I have Apache htpasswd format password store. The fields are ":"
separated and has more fields than standard format. I have script to
read the fields and load into Hash.

I need advice/help to improve the script. The current one works-but
not really good script. The script goes as follow:
--------------------------------
#!/usr/bin/perl
use strict;
use warnings;
Good!

my $data = {};

This is confusing. Is $data a reference to an anonymous hash? The length
of an empty anonymous hash? Are you using curly brackets instead of
regular quotes? Is this a typo and you meant %data instead?

Whatever it is, such a non-standard use is very confusing and also
totally unnecessary because Perl will initialize variables to 'emtpy'
anyway.

for (<DATA>) {

This doesn't make much sense. for() creates the whole argument list
first. So either just read the whole file into a array or use while() to
process it line by line.

my $line = $_;
if ( defined $line ) {

The test is unnecessary. If the line was read from the file then the
variable is defined.

if ( $line =~ /^(.*?).*?).*?).*?).*?)(?:.*))?$/
|| $line =~ /^(.*?).*?).*?).*?)(?:.*))?$/
|| $line =~ /^(.*?).*?).*?)(?:.*))?$/
|| $line =~ /^(.*?).*?)(?:.*))?$/ )

OMG! If your data is colon-separated then just split() it at the colon
and check how many elements the result array has.
@fields = split /:/, $line;
if (@fields > 2 and @fields < 8) {
or something similar.

jue

 

[Peter J. Holzer]

Hello,

I have Apache htpasswd format password store. The fields are ":"
separated and has more fields than standard format. I have script to
read the fields and load into Hash.

I need advice/help to improve the script. The current one works-but
not really good script. The script goes as follow:
--------------------------------
#!/usr/bin/perl
use strict;
use warnings;

my $data = {};

for (<DATA>) {
my $line = $_;

Change these two lines to

if ( defined $line ) {

$line is always defined here so remove this line.

if ( $line =~ /^(.*?).*?).*?).*?).*?)(?:.*))?$/
|| $line =~ /^(.*?).*?).*?).*?)(?:.*))?$/
|| $line =~ /^(.*?).*?).*?)(?:.*))?$/
|| $line =~ /^(.*?).*?)(?:.*))?$/ )
{

That looks extremely convoluted. I didn't even try to understand what
this does. Each line is a colon-separated list of fields, isn't it?
So replace it with:

chomp($line);
my ($username, $pass, $emails, $flag, $pass_change, $flag_change)
= split(/:/, $line);
next unless $pass;

and then use $username, $pass, etc. instead of $1, $2, etc.

$data->{$1}->{pass} = $2;
$data->{$1}->{emails} = $3 || '';
$data->{$1}->{flag} =
( ( defined $4 ) && ( $4 == 0 ) ) ? 0 : ( $4 || '' );

This could be written somewhat more readable as:

$data->{$username}->{flag} = defined $flag ? $flag : '';

or if you have Perl 5.10, as

$data->{$username}->{flag} = $flag // '';

And since this is a flag it should probably only be used in a boolean
context, so it doesn't matter whether the value is 0, '' or undef, so
I would just write

$data->{$username}->{flag} = $flag;

instead.

$data->{$1}->{pass_change} = $5 || '';
$data->{$1}->{flag_change} = $6 || '';

And these are numeric values. 0 may be a valid value, so replacing it
with '' may be an error. Even if it isn't, I'd prefer undef for invalid
values over '', I'd just write

$data->{$username}->{pass_change} = $pass_change;
$data->{$username}->{flag_change} = $flag_change;

here, too.


Finally, the long list of assignments bothers me. So I'd rewrite that to

$data->{$username}
= {
pass => $pass,
emails => $emails || '',
flag => $flag,
pass_change => $pass_change,
flag_change => $flag_change,
};

and the duplication of fieldnames could also be avoided:

my @field_names = qw(pass emails flag pass_change flag_change);
my ($username, @field_values) = split(/:/, $line);
@{ $data->{$username} }{@field_names} = @field_values;

if we can live with an undefined 'emails' field.

}
}

}

use Data:umper;
print Data:umper->Dump( [$data] );

__DATA__
AllPresent:hjliEO35kCgwI:[email protected]:1:23232:24324
LastMissing:CyL92g3OKi.jM:[email protected]:1:2323
FlagZero:CyL92g3OKi.jM:[email protected]:0:23232
OnlyFlag:ZuqpLZ7AxHBvw[email protected]:0
RestMissing:ZuqpLZ7AxHBvw:[email protected]

So my final script looks like this:


#!/usr/bin/perl
use strict;
use warnings;

my $data = {};

while (my $line = <DATA>) {
chomp $line;
my @field_names = qw(pass emails flag pass_change flag_change);
my ($username, @field_values) = split(/:/, $line);
next unless $username;
@{ $data->{$username} }{@field_names} = @field_values;
}

use Data:umper;
print Data:umper->Dump( [$data] );

__DATA__
AllPresent:hjliEO35kCgwI:[email protected]:1:23232:24324
LastMissing:CyL92g3OKi.jM:[email protected]:1:2323
FlagZero:CyL92g3OKi.jM:[email protected]:0:23232
OnlyFlag:ZuqpLZ7AxHBvw[email protected]:0
RestMissing:ZuqpLZ7AxHBvw:[email protected]

hp

 

[Peter J. Holzer]

This is confusing.

I don't think so. It's perfectly clear and normal Perl.

Is $data a reference to an anonymous hash?
Yes.

The length
of an empty anonymous hash?
No.

Are you using curly brackets instead of
regular quotes?

No. Of course he could have meant to write
my $data = q{};
but there is no reason to assume that.

Is this a typo and you meant %data instead?

Then he would also have had to use parentheses instead of the braces.
So this is unlikely.

Whatever it is, such a non-standard use is very confusing and also
totally unnecessary because Perl will initialize variables to 'emtpy'
anyway.

But “empty†(you mean “undefâ€) isn't the same as a reference to an empty
hash. It makes no difference in this program (the anonymous hash is
autovivified at the first access), but an explicit initialization like
this:

* Serves as a reminder that this variable is intended to be used as a
hashref (and not as an arrayref, string, number, or whatever)
* May catch some usage errors
* May prevent autovivification at the wrong place. For example,
my $data; func($data);
is not the same as
my $data = {}; func($data);
if func() accesses $data->{something}.

So it's sometimes necessary, often useful and otherwise harmless.

hp

 

[Dr.Ruud]

But “empty†(you mean “undefâ€) isn't the same as a reference to an empty
hash. It makes no difference in this program (the anonymous hash is
autovivified at the first access), but an explicit initialization like
this:

* Serves as a reminder that this variable is intended to be used as a
hashref (and not as an arrayref, string, number, or whatever)
* May catch some usage errors
* May prevent autovivification at the wrong place. For example,
my $data; func($data);
is not the same as
my $data = {}; func($data);
if func() accesses $data->{something}.

So it's sometimes necessary, often useful and otherwise harmless.

I also, and pretty often, use it in this way:

my $dfbb = $data{ foo }{ bar }{ baz } ||= {};

 

[Dr.Ruud]

my ($username, $pass, $emails, $flag, $pass_change, $flag_change)
= split(/:/, $line);

Variant:

my %line;
@line{qw/ pass emails flag pass_change flag_change /} =
split /:/, $line;

 

[sln]

Hello,

I have Apache htpasswd format password store. The fields are ":"
separated and has more fields than standard format. I have script to
read the fields and load into Hash.

I need advice/help to improve the script. The current one works-but
not really good script. The script goes as follow:
--------------------------------
#!/usr/bin/perl
use strict;
use warnings;

my $data = {};

for (<DATA>) {

my $line = $_;
if ( defined $line ) {
if ( $line =~ /^(.*?).*?).*?).*?).*?)(?:.*))?$/
|| $line =~ /^(.*?).*?).*?).*?)(?:.*))?$/
|| $line =~ /^(.*?).*?).*?)(?:.*))?$/
|| $line =~ /^(.*?).*?)(?:.*))?$/ )
{
$data->{$1}->{pass} = $2;
$data->{$1}->{emails} = $3 || '';
$data->{$1}->{flag} =
( ( defined $4 ) && ( $4 == 0 ) ) ? 0 : ( $4 || '' );
$data->{$1}->{pass_change} = $5 || '';
$data->{$1}->{flag_change} = $6 || '';
}
}

}

use Data:umper;
print Data:umper->Dump( [$data] );

__DATA__
AllPresent:hjliEO35kCgwI:[email protected]:1:23232:24324
LastMissing:CyL92g3OKi.jM:[email protected]:1:2323
FlagZero:CyL92g3OKi.jM:[email protected]:0:23232
OnlyFlag:ZuqpLZ7AxHBvw[email protected]:0
RestMissing:ZuqpLZ7AxHBvw:[email protected]

-------------------------
Please note - only first two fields are must fields, rest may be
missing from lines. first is - username, second is encrypted
password, third is email, fourth is flag to force user to change
password(0-pass change is not required, 1-user must change pass),
fifth- the last time password changed epoch time, sixth- time of
change of flag (some times flag is changed by admins of the system).

thanks in advance

use strict;
use warnings;

my $data = {};

for (<DATA>) {

# ^^ Why for ?

my $line = $_;
if ( defined $line ) {
if (
# $line =~ /^ (.*?) : (.*?) : (.*?) : (.*?) : (.*?) (?: : (.*) ) ? $ /x
# || $line =~ /^ (.*?) : (.*?) : (.*?) : (.*?) (?: : (.*) ) ? $ /x
# || $line =~ /^ (.*?) : (.*?) : (.*?) (?: : (.*) ) ? $ /x
# || $line =~ /^ (.*?) : (.*?) (?: : (.*) ) ? $ /x )

# ^^ Replace with this ->
$line =~ /^ (.*?) .*?) (?:.*?))? (?:.*?))? (?:.*?))? (?:.*))? $ /x

# Here, be more proactive, expect certain class of values with a length
# Should 'if()' fail, send the line to a log file, don't clutter the hash

)
{
# Validate more stuff, at least check that $1 is > ''

$data->{$1}->{pass} = $2;
# ^^ could be '' then key is ''

$data->{$1}->{emails} = $3 || '';
$data->{$1}->{flag} =
( ( defined $4 ) && ( $4 == 0 ) ) ? 0 : ( $4 || '' );
# ^^ What if $4 is alpha-num ? Throws a warning
# Consolidate the above logic !

$data->{$1}->{pass_change} = $5 || '';
$data->{$1}->{flag_change} = $6 || '';
}
}

}

### ^^^^
### Would 'split()' be more affective ??


use Data:umper;
print Data:umper->Dump( [$data] );

__DATA__
AllPresent:hjliEO35kCgwI:[email protected]:1:23232:24324
LastMissing:CyL92g3OKi.jM:[email protected]:1:2323
FlagZero:CyL92g3OKi.jM:[email protected]:0:23232
OnlyFlag:ZuqpLZ7AxHBvw[email protected]:0
RestMissing:ZuqpLZ7AxHBvw:[email protected]
:::asdf:

-sln

 

[sln]

Hello,

I have Apache htpasswd format password store. The fields are ":"
separated and has more fields than standard format. I have script to
read the fields and load into Hash.

I need advice/help to improve the script. The current one works-but
not really good script. The script goes as follow:
--------------------------------
#!/usr/bin/perl
use strict;
use warnings;

my $data = {};

for (<DATA>) {

my $line = $_;
if ( defined $line ) {
if ( $line =~ /^(.*?).*?).*?).*?).*?)(?:.*))?$/
|| $line =~ /^(.*?).*?).*?).*?)(?:.*))?$/
|| $line =~ /^(.*?).*?).*?)(?:.*))?$/
|| $line =~ /^(.*?).*?)(?:.*))?$/ )
{
$data->{$1}->{pass} = $2;
$data->{$1}->{emails} = $3 || '';
$data->{$1}->{flag} =
( ( defined $4 ) && ( $4 == 0 ) ) ? 0 : ( $4 || '' );
$data->{$1}->{pass_change} = $5 || '';
$data->{$1}->{flag_change} = $6 || '';
}
}

}

use Data:umper;
print Data:umper->Dump( [$data] );

__DATA__
AllPresent:hjliEO35kCgwI:[email protected]:1:23232:24324
LastMissing:CyL92g3OKi.jM:[email protected]:1:2323
FlagZero:CyL92g3OKi.jM:[email protected]:0:23232
OnlyFlag:ZuqpLZ7AxHBvw[email protected]:0
RestMissing:ZuqpLZ7AxHBvw:[email protected]

-------------------------
Please note - only first two fields are must fields, rest may be
missing from lines. first is - username, second is encrypted
password, third is email, fourth is flag to force user to change
password(0-pass change is not required, 1-user must change pass),
fifth- the last time password changed epoch time, sixth- time of
change of flag (some times flag is changed by admins of the system).

thanks in advance

use strict;
use warnings;

my $data = {};

for (<DATA>) {

[snip]

Oh, and the array primary keys are suseptible to bein overwritten as your
list processes.

-sln

 

[sln]

I also, and pretty often, use it in this way:

my $dfbb = $data{ foo }{ bar }{ baz } ||= {};

If I have to spend more than 3 seconds understanding a line of
Perl that isin't a regular expression, I pass it by.

-sln

 

[sopan.shewale]

Appreciate every one for responding to this question.

The solution with split is also good idea. I am sticking to following
solution.

------------------------

#!/usr/bin/perl
use strict;
use warnings;

my $data = {};

for (<DATA>) {
chomp;
my $line = $_;

if ( defined $line ) {
if ( $line =~ /^([^:]*)[^:]*)[^:]*)(?:[^:]*)(?:[^:]*)
(?:[^:]*)(?:.*))?)?)?)?$/ )
{

$data->{$1}->{pass} = $2;
# ^^ $1 will never be '' or undef...
# in that case there is issue with password line
$data->{$1}->{emails} = $3 || '';
$data->{$1}->{flag} =
( ( defined $4 ) && ( $4 == 0 ) ) ? 0 : ( $4 || '' );
# we are assumming $4 as 0 or 1
$data->{$1}->{pass_change} = $5 || '';
$data->{$1}->{flag_change} = $6 || '';
}
}

}

use Data:umper;
print Data:umper->Dump( [$data] );

__DATA__
AllPresent:hjliEO35kCgwI:[email protected]:1:23232:24324
LastMissing:CyL92g3OKi.jM:[email protected]:1:2323
FlagZero:CyL92g3OKi.jM:[email protected]:0:23232
OnlyFlag:ZuqpLZ7AxHBvw[email protected]:0
RestMissing:ZuqpLZ7AxHBvw:[email protected]

 

[Uri Guttman]

ssc> Appreciate every one for responding to this question.
ssc> The solution with split is also good idea. I am sticking to following
ssc> solution.

why would you stick with this complex solution over the much simpler and
faster split?

ssc> if ( defined $line ) {
ssc> if ( $line =~ /^([^:]*)[^:]*)[^:]*)(?:[^:]*)(?:[^:]*)
ssc> (?:[^:]*)(?:.*))?)?)?)?$/ )

ok, here is a test. explain what that line is doing in 30 words or
less. if you can't explain a single line of code like that, it is overly
complex code. note that the ? and : chars both play two different roles
(and many dupes of them). that is NOT a good solution. just my strong
opinion.

uri

 

[Dr.Ruud]

Appreciate every one for responding to this question.

The solution with split is also good idea. I am sticking to following
solution.

------------------------

#!/usr/bin/perl
use strict;
use warnings;

my $data = {};

for (<DATA>) {

Bad, as you have been told several times.

chomp;
my $line = $_;

That $line should be set in the while-line starting the loop.

if ( defined $line ) {
if ( $line =~ /^([^:]*)[^:]*)[^:]*)(?:[^:]*)(?:[^:]*)
(?:[^:]*)(?:.*))?)?)?)?$/ )

Butt ugly.

{

$data->{$1}->{pass} = $2;
# ^^ $1 will never be '' or undef...
# in that case there is issue with password line
$data->{$1}->{emails} = $3 || '';
$data->{$1}->{flag} =
( ( defined $4 ) && ( $4 == 0 ) ) ? 0 : ( $4 || '' );
# we are assumming $4 as 0 or 1
$data->{$1}->{pass_change} = $5 || '';
$data->{$1}->{flag_change} = $6 || '';

Silly '||' usage. Copy/paste coding style.

}
}

}

use Data:umper;

Put that line in the top, because that is where it is.

print Data:umper->Dump( [$data] );

__DATA__
AllPresent:hjliEO35kCgwI:[email protected]:1:23232:24324
LastMissing:CyL92g3OKi.jM:[email protected]:1:2323
FlagZero:CyL92g3OKi.jM:[email protected]:0:23232
OnlyFlag:ZuqpLZ7AxHBvw[email protected]:0
RestMissing:ZuqpLZ7AxHBvw:[email protected]
-----------------------------------------------------------

I got the help on regex from Peter Thoeny (http://twiki.org)
The part of this code is checked-in into TWiki repository
thanks every one for helping

You are publishing bad quality code. I expect you
to clearly mark it as such, so visitors know what they are getting.


#!/usr/bin/perl -wl
use strict;

use Data:umper;

my @cols = qw/ pass emails flag pass_change flag_change /;
my %data;

while ( <DATA> ) {
next if /^\s*(?:$|#)/; # skip empty and comment lines
chomp;
my @line = split /:/;
my $key = shift @line or die "Bad data at line $.\n";
@{ $data{ $key } }{ @cols } = @line;
$data{ $key }{ flag } ||= 0; # or use //=
}

print Dumper \%data;


__DATA__
AllPresent:hjliEO35kCgwI:[email protected]:1:23232:24324
LastMissing:CyL92g3OKi.jM:[email protected]:1:2323
FlagZero:CyL92g3OKi.jM:[email protected]:0:23232
OnlyFlag:ZuqpLZ7AxHBvw[email protected]:0
RestMissing:ZuqpLZ7AxHBvw:[email protected]

0 :
#EOF

 

[Jürgen Exner]

Appreciate every one for responding to this question.

The solution with split is also good idea. I am sticking to following
solution. [...]
for (<DATA>) {

chomp;
my $line = $_;

Why are you sticking to the separate assignment?

if ( defined $line ) {

Why are you sticking to the useless defined() test?

if ( $line =~ /^([^:]*)[^:]*)[^:]*)(?:[^:]*)(?:[^:]*)
(?:[^:]*)(?:.*))?)?)?)?$/ )

Why are you sticking to the complex RE when a split() is so much easier?

jue

 

[sopan.shewale]

Bad, as you have been told several times.

[sopan] - yup, bad. correcting it.
The one posting here in groups is just logic. In actual code
at TWiki, its while loop.
The data comes from the file from filesystem, opened with
standard functions/else thrown error with modules like Error::Simple.

That $line should be set in the while-line starting the loop.

[sopan] - yes, agree.

    if ( defined $line ) {
        if ( $line =~ /^([^:]*)[^:]*)[^:]*)(?:[^:]*)(?:[^:]*)
(?:[^:]*)(?:.*))?)?)?)?$/ )

Butt ugly.

[sopan] - yes, its ugly. decided to move to split solution (the
original code of my package had similar lines for three field password
file since long time. But no complains).

Silly '||' usage. Copy/paste coding style.

[sopan] - yes,it was copy+paste . Moving to better style. Thank you,

Put that line in the top, because that is where it is.

[sopan] - it was just for testing purposes. These lines are not
present in code.But yes, in forums also, whatever i pasted, i should
have pasted better and agree to have this line at the upper part of
code. my mistake.

print Data:umper->Dump( [$data] );

__DATA__
AllPresent:hjliEO35kCgwI:[email protected]:1:23232:24324
LastMissing:CyL92g3OKi.jM:[email protected]:1:2323
FlagZero:CyL92g3OKi.jM:[email protected]:0:23232
OnlyFlag:ZuqpLZ7AxHBvw[email protected]:0
RestMissing:ZuqpLZ7AxHBvw:[email protected]
-----------------------------------------------------------

I got the help on regex from Peter Thoeny (http://twiki.org)
The part of this code is checked-in into TWiki repository
thanks every one for helping

You are publishing bad quality code. I expect you
to clearly mark it as such, so visitors know what they are getting.

#!/usr/bin/perl -wl
use strict;

use Data:umper;

my @cols = qw/ pass emails flag pass_change flag_change /;
my %data;

while ( <DATA> ) {
     next if /^\s*(?:$|#)/;  # skip empty and comment lines
     chomp;
     my @line = split /:/;
     my $key = shift @line or die "Bad data at line $.\n";
     @{ $data{ $key } }{ @cols } = @line;
     $data{ $key }{ flag } ||= 0;  # or use //=

}

print Dumper \%data;

__DATA__
AllPresent:hjliEO35kCgwI:[email protected]:1:23232:24324
LastMissing:CyL92g3OKi.jM:[email protected]:1:2323
FlagZero:CyL92g3OKi.jM:[email protected]:0:23232
OnlyFlag:ZuqpLZ7AxHBvw[email protected]:0
RestMissing:ZuqpLZ7AxHBvw:[email protected]

0 :
#EOF

[sopan] - thank you so much Ruud. this was the best version.

 

[sopan.shewale]

Appreciate every one for responding to this question.

The solution with split is also good idea. I am sticking to following
solution. [...]
for (<DATA>) {

Why are you sticking with a for(<>) loop? It doesn't have any advantage
over a while() loop or slurping the file into a variable.

[sopan] - in actual code which goes in package has while. The posted
here was just part of code. Its while, agree with you to have while
instead of for. thank you,

Why are you sticking to the separate assignment?

[sopan] -yes, makes sense to have my $line = $_; chomp $line;

Why are you sticking to the useless defined() test?

[sopan] agree, not required to have "defined $line"

       if ( $line =~ /^([^:]*)[^:]*)[^:]*)(?:[^:]*)(?:[^:]*)
(?:[^:]*)(?:.*))?)?)?)?$/ )

Why are you sticking to the complex RE when a split() is so much easier?

[sopan] moving to split based stuff. agree its bad looking
expression.

jue

[sopan] Thanks jue

 

[sopan.shewale]

  ssc> Appreciate every one for responding to this question.
  ssc> The solution with split is also good idea. I am sticking to following
  ssc> solution.

why would you stick with this complex solution over the much simpler and
faster split?

[sopan] - moving with split, thank you

  ssc>     if ( defined $line ) {
  ssc>         if ( $line =~ /^([^:]*)[^:]*)[^:]*)(?:[^:]*)(?:[^:]*)
  ssc> (?:[^:]*)(?:.*))?)?)?)?$/ )

ok, here is a test. explain what that line is doing in 30 words or
less. if you can't explain a single line of code like that, it is overly
complex code. note that the ? and : chars both play two different roles
(and many dupes of them). that is NOT a good solution. just my strong
opinion.

[sopan] - thanks uri for strong advice. [1] difficult to explain [2]
30 or less words-just impossible. Actually similar code has been with
the package for long times - but no complain. i will move to split
which is clean solution.

 

[Martijn Lievaart]

Appreciate every one for responding to this question.

The solution with split is also good idea. I am sticking to following
solution. [...]
for (<DATA>) {

Why are you sticking with a for(<>) loop? It doesn't have any advantage
over a while() loop or slurping the file into a variable.

[sopan] - in actual code which goes in package has while. The posted
here was just part of code. Its while, agree with you to have while
instead of for. thank you,

Why are you sticking to the separate assignment?

[sopan] -yes, makes sense to have my $line = $_; chomp $line;

Why are you sticking to the useless defined() test?

[sopan] agree, not required to have "defined $line"

       if ( $line =~
       /^([^:]*)[^:]*)[^:]*)(?:[^:]*)(?:[^:]*)
(?:[^:]*)(?:.*))?)?)?)?$/ )

Why are you sticking to the complex RE when a split() is so much
easier?

[sopan] moving to split based stuff. agree its bad looking expression.

Again, it can be as easy as this:

#!/usr/bin/perl
use strict;
use warnings;

my %data;

while (my $line = <DATA>) {

# chop of newline
chomp $line;

# split into fields and stuff into hash, using a hash slice
@{$data{$user}}{qw/pass email flag passchange flagchange/} =
split /:/, $line, 6;
}

OK, you get undef for every field not defined, but that usually is what
you want.

M4

 

[sopan.shewale]

Appreciate every one for responding to this question.
The solution with split is also good idea. I am sticking to following
solution.
[...]
for (<DATA>) {
Why are you sticking with a for(<>) loop? It doesn't have any advantage
over a while() loop or slurping the file into a variable.

[sopan] - in actual code which goes in package has while. The posted
here was just part of code. Its while, agree with you to have while
instead of for. thank you,

   chomp;
   my $line = $_;
Why are you sticking to the separate assignment?

[sopan] -yes, makes sense to have my $line = $_; chomp $line;

   if ( defined $line )
Why are you sticking to the useless defined() test?

[sopan] agree, not required to have "defined $line"

       if ( $line =~
       /^([^:]*)[^:]*)[^:]*)(?:[^:]*)(?:[^:]*)
(?:[^:]*)(?:.*))?)?)?)?$/ )
Why are you sticking to the complex RE when a split() is so much
easier?

[sopan] moving to split based stuff. agree its bad looking expression.

Again, it can be as easy as this:

#!/usr/bin/perl
use strict;
use warnings;

my %data;

while (my $line = <DATA>) {

    # chop of newline
    chomp $line;

    # split into fields and stuff into hash, using a hash slice
    @{$data{$user}}{qw/pass email flag passchange flagchange/} =
        split /:/, $line, 6;

}

OK, you get undef for every field not defined, but that usually is what
you want.

M4

This definitely works - but wanted to take care of flag to have 0 if
its not defined. Not sure where and how many places %data is used in
the whole package.

Thank you

 

[sln]

Appreciate every one for responding to this question.

The solution with split is also good idea. I am sticking to following
solution.

------------------------

#!/usr/bin/perl
use strict;
use warnings;

my $data = {};

for (<DATA>) {
chomp;
my $line = $_;

if ( defined $line ) {
if ( $line =~ /^([^:]*)[^:]*)[^:]*)(?:[^:]*)(?:[^:]*)
(?:[^:]*)(?:.*))?)?)?)?$/ )

^^^^^^^
The pattern doesn't change in form, therefore this is not necessary.
The switch from .*? to [^:]* was good, but why the final .*

{

$data->{$1}->{pass} = $2;
# ^^ $1 will never be '' or undef...
# in that case there is issue with password line
$data->{$1}->{emails} = $3 || '';
$data->{$1}->{flag} =
( ( defined $4 ) && ( $4 == 0 ) ) ? 0 : ( $4 || '' );
# we are assumming $4 as 0 or 1
$data->{$1}->{pass_change} = $5 || '';
$data->{$1}->{flag_change} = $6 || '';
}
}

}

use Data:umper;
print Data:umper->Dump( [$data] );

__DATA__
AllPresent:hjliEO35kCgwI:[email protected]:1:23232:24324
LastMissing:CyL92g3OKi.jM:[email protected]:1:2323
FlagZero:CyL92g3OKi.jM:[email protected]:0:23232
OnlyFlag:ZuqpLZ7AxHBvw[email protected]:0
RestMissing:ZuqpLZ7AxHBvw:[email protected]
-----------------------------------------------------------

I got the help on regex from Peter Thoeny (http://twiki.org)
The part of this code is checked-in into TWiki repository
thanks every one for helping

I just got to scratch my head on this one.
If you can't use split, use a regex like below that will assign
'' to shorter lines. That way you don't have to do all that
define(d) checking.

-sln

------------------------
use strict;
use warnings;
use Data:umper;

my $data = {};

while (defined (my $line = <DATA>)) {
chomp $line;
$line =~ /^
([^:]*)
: (?<pass> [^:]*)
: (?<emails> [^:]*)
:? (?<flag> [^:]*)
:? (?<pass_change> [^:]*)
:? (?<flag_change> .*)
$/x
and $data->{$1} = {%+};
}

print Dumper($data);

__DATA__
AllPresent:hjliEO35kCgwI:[email protected]:1:23232:24324
LastMissing:CyL92g3OKi.jM:[email protected]:1:2323
FlagZero:CyL92g3OKi.jM:[email protected]:0:23232
OnlyFlag:ZuqpLZ7AxHBvw[email protected]:0
RestMissing:ZuqpLZ7AxHBvw:[email protected]