This node falls below the community's minimum standard of quality and will not be displayed.

Replies are listed 'Best First'.
Re: String concatenation function
by TVSET (Chaplain) on Jun 01, 2003 at 20:55 UTC
Re: String concatenation function
by halley (Prior) on Jun 01, 2003 at 23:09 UTC
    Okay, the others have already stated that there's an operator which does what your function does. That's all a part of learning. And it is clear that you're just learing Perl. Welcome. In that vein, with no offense intended, let me go through your script with some other comments.
    ############################### # STRING CONCAT FUNCTION # # BY DR JONATHAN REES # ###############################
    I find "box comments" to be a waste of time. The all-capitals also are harder to read than plain English... this looks like some old FORTRAN-66 code.
    $l = 5; # $l defines the number of asterisks
    You probably shouldn't assign to $1, $2, or so on. These special variables receive the results of regular expression captures. But maybe that's a lowercase 'ell' letter. You might avoid single letter variables too, especially ones that look like 0 O 1 l...
    &concat($l); # calls the sub concat passing the # number of concatenations in $l
    Pick your syntax: either &concat or concat(...). Combining them is redundant and may confuse someone maintaining your code.

    Also, don't describe the effect of every statement with a comment. Comments are not to explain what the statement does, they're to explain what the algorithm does. A year from now when you're very quick with Perl, the transliteral comments will just be distracting noise. Put strategy in comments, tactics in code.

    print $i; # prints the result
    Another example of transliteral comments. And where did $i come from? It would seem much more natural to have the function return a value and for the caller to catch the value in whatever variable they choose.

    And try to end your last printout with a newline. That's either the default value of $/, or a literal "\n".

    sub concat { ... }
    Pick an indentation style that helps people read your code. If the closing curly brace was in column 1, people could see it was related to the sub statement.
    my $val = $_[0] + 1; for ($k = 1 ; $k < $val ; $k++){ $i = $i.$sym; }
    Firstly, you seem to assume that $i will be empty when you call the function... what happens when you call this function twice?

    Secondly, Perl numbers its arrays from zero, as you've discovered here. But you seem to think of iteration as starting with one. This isn't a crime, but you seem to have stumbled upon a loop strategy that gets you the right number of iterations, quite accidentally. Since the number in $k isn't useful to the loop, I'd suggest:

    my $i = ''; for (1 .. $_[0]) { $i = $i . $sym }
    This leaves us with the following code (assuming there was no such  'abc' x 5 operation):
    # Concatenate a number of symbols. print concat('abc', 5), $/; sub concat { my $i = ''; for (1 .. $_[1]) { $i = $i . $_[0] } return $i; }

    --
    [ e d @ h a l l e y . c c ]

      You probably shouldn't assign to $1, $2, or so on.

      You can't. This is indeed lowercase L.

      You might avoid single letter variables too, especially ones that look like 0 O 1 l...

      And you might want to use a font that doesn't render different things characters the same.

      Pick your syntax: either &concat or concat(...). Combining them is redundant and may confuse someone maintaining your code.

      &foo without parens and arguments is scary (implicitly passes @_). This is *exactly* why you shouldn't use the &-form unless you know what you are doing. Style is not important here.

      This leaves us with the following code (assuming there was no such 'abc' x 5 operation):

      One direct access of an @_ element is okay, but once you start using more, in my opinion you should assign the arguments to variables. And still, $i .= $foo is clearer and less work than $i = $i . $foo;

      Juerd # { site => 'juerd.nl', plp_site => 'plp.juerd.nl', do_not_use => 'spamtrap' }

        Thanks for all your help! Learning fast...
Re: String concatenation function
by Limbic~Region (Chancellor) on Jun 01, 2003 at 20:55 UTC
    jonnyr9,
    What does this do that the x operator doesn't do? Have you looked at perldoc perlop. This is the string multiplier.
    #!/usr/bin/perl -w use strict; my $string = '*'; $string = $string x 5; print "$string\n";
    L~R
Re: String concatenation function
by Juerd (Abbot) on Jun 01, 2003 at 22:56 UTC

    # BY DR JONATHAN REES #

    Don't be too proud.

    $sym = "*"; # $sym is the string to be concatenated

    Why is that not a parameter?

    &concat($l); # calls the sub concat passing the

    Don't use "&" when calling a sub. It's old fashioned and if you don't know its meaning (I doubt you do), you probably shouldn't be using it.

    print $i; # prints the result

    Don't comment *what* your code does, comment *why* it does that.

    for ($k = 1 ; $k < $val ; $k++){ $i = $i.$sym; }

    C-style for is error prone and a lot of work. Perlish foreach is more efficient and easier. The ".=" operator would have saved you two more non-whitespace characters. Besides that, you should of course have used the "x" operator.

    }

    And fix your indenting logic.

    Juerd # { site => 'juerd.nl', plp_site => 'plp.juerd.nl', do_not_use => 'spamtrap' }

      Don't use "&" when calling a sub. It's old fashioned and if you don't know its meaning (I doubt you do), you probably shouldn't be using it.

      Do *you* know what it does? If so, then your advice surprises me. What it does is prevent a Perl built-in from being called if your subroutine name happens to collide with the name of a Perl built-in. Why should anyone avoid that protection?

      Quite the contrary, using & (and parentheses) to call your own subroutines is prudent style, especially if you don't name your subroutines with mixed case and don't care to memorize all of the current Perl keywords (some rather obscure) and predict possible future keywods (and monitor actual creation of new keywords and search all of your scripts when this happens and modify any that now require modification).

      It can also be a nice style by visually distinguishing calls to built-ins from calls to user-defined subroutines (which can aid those reading the code by helping them figure out where to find more information on the named routine).

      It is good advice to avoid using & without parens to call a subroutine. But that isn't what was done and you didn't even mention this problem. Also note that you should use & without parens with defined or to get a reference to a named subroutine.

                      - tye

        Do *you* know what it does? If so, then your advice surprises me. What it does is prevent a Perl built-in from being called if your subroutine name happens to collide with the name of a Perl built-in. Why should anyone avoid that protection?

        & disables prototypes and inlining. If used without parens and arguments, it implicitly passes @_. That alone should be a good reason for telling newbies to avoid using & when calling subs, or to always use parens. I think it's much better to simply say that & is not to be used unless you know why. Same goes for symbolic references, local (actually, package global variables in general), steak knifes and bicycles.

        Calling a sub without the ampersand is faster. This doesn't matter of course, but perhaps this shows how the newer style is optimized.

        Quoting a usenet post by tchrist:

        (...) You can spot a an old, perl4ish, non-ORA, and/or non-lwallian book a league or seven away by its obsequious allegiance to an ampersand.

        Quoting perlsub:

        Not only does the "&" form make the argument list optional, it also disables any prototype checking on arguments you do provide. This is partly for historical reasons, and partly for having a convenient way to cheat if you know what you're doing. See Prototypes below.

        (...)

        (...) If you call it like an old-fashioned subroutine, then it behaves like an old-fashioned subroutine. (...)

        Quoting perlstyle:

        (...) Call your subroutines as if they were functions or list operators to avoid excessive ampersands and parentheses.

        Also note that you should use & without parens with defined or to get a reference to a named subroutine.

        I explicitly discussed *calling* subs with &. You do of course need it when you syntactically treat the sub as a variable.

        Juerd # { site => 'juerd.nl', plp_site => 'plp.juerd.nl', do_not_use => 'spamtrap' }