Need help/advice to improve script

Discussion in 'Perl Misc' started by sopan.shewale@gmail.com, Jul 21, 2010.

  1. Guest

    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::Dumper;
    print Data::Dumper->Dump( [$data] );

    __DATA__
    AllPresent:hjliEO35kCgwI::1:23232:24324
    LastMissing:CyL92g3OKi.jM::1:2323
    FlagZero:CyL92g3OKi.jM::0:23232
    OnlyFlag:ZuqpLZ7AxHBvw:eek::0
    RestMissing:ZuqpLZ7AxHBvw:

    -------------------------
    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
    , Jul 21, 2010
    #1
    1. Advertising

  2. On Wed, 21 Jul 2010 02:17:20 -0700, wrote:

    > 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>) {


    While (<...>) has magic, use it.

    >
    > 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::Dumper;
    print Data::Dumper->Dump( [\%data] );

    __DATA__
    AllPresent:hjliEO35kCgwI::1:23232:24324
    LastMissing:CyL92g3OKi.jM::1:2323
    FlagZero:CyL92g3OKi.jM::0:23232
    OnlyFlag:ZuqpLZ7AxHBvw:eek::0
    RestMissing:ZuqpLZ7AxHBvw:


    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
    Martijn Lievaart, Jul 21, 2010
    #2
    1. Advertising

  3. "" <> wrote:
    >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
    Jürgen Exner, Jul 21, 2010
    #3
  4. On 2010-07-21 09:17, <> wrote:
    > 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

    while (my $line = <DATA>) {


    > 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::Dumper;
    > print Data::Dumper->Dump( [$data] );
    >
    > __DATA__
    > AllPresent:hjliEO35kCgwI::1:23232:24324
    > LastMissing:CyL92g3OKi.jM::1:2323
    > FlagZero:CyL92g3OKi.jM::0:23232
    > OnlyFlag:ZuqpLZ7AxHBvw:eek::0
    > RestMissing:ZuqpLZ7AxHBvw:
    >


    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::Dumper;
    print Data::Dumper->Dump( [$data] );

    __DATA__
    AllPresent:hjliEO35kCgwI::1:23232:24324
    LastMissing:CyL92g3OKi.jM::1:2323
    FlagZero:CyL92g3OKi.jM::0:23232
    OnlyFlag:ZuqpLZ7AxHBvw:eek::0
    RestMissing:ZuqpLZ7AxHBvw:

    hp
    Peter J. Holzer, Jul 21, 2010
    #4
  5. On 2010-07-21 12:31, Jürgen Exner <> wrote:
    > "" <> wrote:
    >>my $data = {};

    >
    > 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
    Peter J. Holzer, Jul 21, 2010
    #5
  6. Dr.Ruud Guest

    Peter J. Holzer wrote:

    > 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 } ||= {};

    --
    Ruud
    Dr.Ruud, Jul 21, 2010
    #6
  7. Dr.Ruud Guest

    Peter J. Holzer wrote:

    > 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;

    --
    Ruud
    Dr.Ruud, Jul 21, 2010
    #7
  8. Guest

    On Wed, 21 Jul 2010 02:17:20 -0700 (PDT), "" <> wrote:

    >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::Dumper;
    >print Data::Dumper->Dump( [$data] );
    >
    >__DATA__
    >AllPresent:hjliEO35kCgwI::1:23232:24324
    >LastMissing:CyL92g3OKi.jM::1:2323
    >FlagZero:CyL92g3OKi.jM::0:23232
    >OnlyFlag:ZuqpLZ7AxHBvw:eek::0
    >RestMissing:ZuqpLZ7AxHBvw:
    >
    >-------------------------
    >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::Dumper;
    print Data::Dumper->Dump( [$data] );

    __DATA__
    AllPresent:hjliEO35kCgwI::1:23232:24324
    LastMissing:CyL92g3OKi.jM::1:2323
    FlagZero:CyL92g3OKi.jM::0:23232
    OnlyFlag:ZuqpLZ7AxHBvw:eek::0
    RestMissing:ZuqpLZ7AxHBvw:
    :::asdf:

    -sln
    , Jul 22, 2010
    #8
  9. Guest

    On Wed, 21 Jul 2010 16:30:43 -0700, wrote:

    >On Wed, 21 Jul 2010 02:17:20 -0700 (PDT), "" <> wrote:
    >
    >>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::Dumper;
    >>print Data::Dumper->Dump( [$data] );
    >>
    >>__DATA__
    >>AllPresent:hjliEO35kCgwI::1:23232:24324
    >>LastMissing:CyL92g3OKi.jM::1:2323
    >>FlagZero:CyL92g3OKi.jM::0:23232
    >>OnlyFlag:ZuqpLZ7AxHBvw:eek::0
    >>RestMissing:ZuqpLZ7AxHBvw:
    >>
    >>-------------------------
    >>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
    , Jul 22, 2010
    #9
  10. Guest

    On Wed, 21 Jul 2010 19:54:36 +0200, "Dr.Ruud" <> wrote:

    >Peter J. Holzer wrote:
    >
    >> 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 } ||= {};


    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
    , Jul 22, 2010
    #10
  11. Guest

    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::Dumper;
    print Data::Dumper->Dump( [$data] );

    __DATA__
    AllPresent:hjliEO35kCgwI::1:23232:24324
    LastMissing:CyL92g3OKi.jM::1:2323
    FlagZero:CyL92g3OKi.jM::0:23232
    OnlyFlag:ZuqpLZ7AxHBvw:eek::0
    RestMissing:ZuqpLZ7AxHBvw:
    -----------------------------------------------------------

    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
    , Jul 22, 2010
    #11
  12. Uri Guttman Guest

    >>>>> "ssc" == sopan shewale@gmail com <> writes:

    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

    --
    Uri Guttman ------ -------- http://www.sysarch.com --
    ----- Perl Code Review , Architecture, Development, Training, Support ------
    --------- Gourmet Hot Cocoa Mix ---- http://bestfriendscocoa.com ---------
    Uri Guttman, Jul 22, 2010
    #12
  13. Dr.Ruud Guest

    wrote:
    > 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::Dumper;


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



    > print Data::Dumper->Dump( [$data] );
    >
    > __DATA__
    > AllPresent:hjliEO35kCgwI::1:23232:24324
    > LastMissing:CyL92g3OKi.jM::1:2323
    > FlagZero:CyL92g3OKi.jM::0:23232
    > OnlyFlag:ZuqpLZ7AxHBvw:eek::0
    > RestMissing:ZuqpLZ7AxHBvw:
    > -----------------------------------------------------------
    >
    > 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::Dumper;

    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::1:23232:24324
    LastMissing:CyL92g3OKi.jM::1:2323
    FlagZero:CyL92g3OKi.jM::0:23232
    OnlyFlag:ZuqpLZ7AxHBvw:eek::0
    RestMissing:ZuqpLZ7AxHBvw:

    0 :
    #EOF


    --
    Ruud
    Dr.Ruud, Jul 22, 2010
    #13
  14. "" <> wrote:
    >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.

    > 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
    Jürgen Exner, Jul 22, 2010
    #14
  15. Guest

    On Jul 22, 1:10 pm, "Dr.Ruud" <> wrote:
    > wrote:
    > > 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.
    >


    [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.


    > >     chomp;
    > >     my $line = $_;

    >
    > 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).

    > >  {

    >
    > >             $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.
    >

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

    > >         }
    > >     }

    >
    > > }

    >
    > > use Data::Dumper;

    >
    > 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::Dumper->Dump( [$data] );

    >
    > > __DATA__
    > > AllPresent:hjliEO35kCgwI::1:23232:24324
    > > LastMissing:CyL92g3OKi.jM::1:2323
    > > FlagZero:CyL92g3OKi.jM::0:23232
    > > OnlyFlag:ZuqpLZ7AxHBvw:eek::0
    > > RestMissing:ZuqpLZ7AxHBvw:
    > > -----------------------------------------------------------

    >
    > > 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::Dumper;
    >
    > 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::1:23232:24324
    > LastMissing:CyL92g3OKi.jM::1:2323
    > FlagZero:CyL92g3OKi.jM::0:23232
    > OnlyFlag:ZuqpLZ7AxHBvw:eek::0
    > RestMissing:ZuqpLZ7AxHBvw:
    >
    > 0 :
    > #EOF
    >
    > --
    > Ruud


    [sopan] - thank you so much Ruud. this was the best version.
    , Jul 22, 2010
    #15
  16. Guest

    On Jul 22, 1:20 pm, Jürgen Exner <> wrote:
    > "" <> wrote:
    > >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.



    > jue



    [sopan] Thanks jue
    , Jul 22, 2010
    #16
  17. Guest

    On Jul 22, 10:43 am, "Uri Guttman" <> wrote:
    > >>>>> "ssc" == sopan shewale@gmail com <> writes:

    >
    >   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.


    > uri
    >
    > --
    > Uri Guttman  ------    --------  http://www.sysarch.com--
    > -----  Perl Code Review , Architecture, Development, Training, Support ------
    > ---------  Gourmet Hot Cocoa Mix  ----  http://bestfriendscocoa.com---------
    , Jul 22, 2010
    #17
  18. On Thu, 22 Jul 2010 02:07:56 -0700, wrote:

    > On Jul 22, 1:20 pm, Jürgen Exner <> wrote:
    >> "" <> wrote:
    >> >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
    Martijn Lievaart, Jul 22, 2010
    #18
  19. Guest

    On Jul 22, 5:34 pm, Martijn Lievaart <> wrote:
    > On Thu, 22 Jul 2010 02:07:56 -0700, wrote:
    > > On Jul 22, 1:20 pm, Jürgen Exner <> wrote:
    > >> "" <> wrote:
    > >> >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
    , Jul 22, 2010
    #19
  20. Guest

    On Wed, 21 Jul 2010 22:08:22 -0700 (PDT), "" <> wrote:

    >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::Dumper;
    >print Data::Dumper->Dump( [$data] );
    >
    >__DATA__
    >AllPresent:hjliEO35kCgwI::1:23232:24324
    >LastMissing:CyL92g3OKi.jM::1:2323
    >FlagZero:CyL92g3OKi.jM::0:23232
    >OnlyFlag:ZuqpLZ7AxHBvw:eek::0
    >RestMissing:ZuqpLZ7AxHBvw:
    >-----------------------------------------------------------
    >
    >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::Dumper;

    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::1:23232:24324
    LastMissing:CyL92g3OKi.jM::1:2323
    FlagZero:CyL92g3OKi.jM::0:23232
    OnlyFlag:ZuqpLZ7AxHBvw:eek::0
    RestMissing:ZuqpLZ7AxHBvw:
    , Jul 22, 2010
    #20
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. gbattine
    Replies:
    8
    Views:
    427
    Philipp Leitner
    Jul 16, 2006
  2. Jari Aalto
    Replies:
    4
    Views:
    474
    Robert Kern
    Oct 15, 2005
  3. Matthew Wilson

    Need advice on how to improve this function

    Matthew Wilson, Aug 20, 2006, in forum: Python
    Replies:
    3
    Views:
    284
    Gabriel Genellina
    Aug 22, 2006
  4. jdm
    Replies:
    4
    Views:
    351
  5. George Orwell

    Silly li'l perl script, plx improve.

    George Orwell, Jan 4, 2007, in forum: Perl Misc
    Replies:
    5
    Views:
    140
    Eric Schwartz
    Jan 4, 2007
Loading...

Share This Page