Please Criticize My Code

Discussion in 'Python' started by Ray, Aug 20, 2005.

  1. Ray

    Ray Guest

    Hello,

    I just wrote a short script that generates look and say sequence. What
    do you Python guys think of it? Too "Java-ish"? I can't shake off the
    feeling that somebody may have done this with 2-3 lines of Python
    magic. Heh.

    # generator for sequence
    def lookAndSaySequence(firstTerm, n):
    term = str(firstTerm)
    for i in xrange(n):
    yield term
    term = lookAndSay(term)

    # the method that looks, and says
    def lookAndSay(number):
    look_at_this = str(number)
    say_this = ['0', look_at_this[0]]
    for digit in look_at_this:
    if say_this[-1] != digit:
    say_this.extend(('1', digit))
    else:
    say_this[-2] = str(int(say_this[-2]) + 1)
    return "".join(say_this)

    # run it!
    print [x for x in lookAndSaySequence(1, 30)]

    Thanks,
    Ray
     
    Ray, Aug 20, 2005
    #1
    1. Advertising

  2. Ray wrote:

    > I just wrote a short script that generates look and say sequence. What
    > do you Python guys think of it? Too "Java-ish"?


    Yes, but that's beside the point. :)

    I think your basic design was sound enough for this application
    (presumably this isn't something that needs to run at high speed). I
    found it a little hard to understand what was going on. Part of this was
    due to variable and function naming choices. Converting between strs and
    ints all the time is confusing too.

    I think this version might be a little easier to understand, and
    describe() should work for any string, not just integers. I think your
    original version does as well:

    def describe(string):
    last_char = ""
    last_count = ""
    res = []

    for char in string:
    if char == last_char:
    last_count += 1
    else:
    res.extend([str(last_count), last_char])
    last_char = char
    last_count = 1

    res.extend([str(last_count), last_char])

    return "".join(res)

    def describe_generator(start, count):
    string = str(start)

    for index in xrange(count):
    yield string
    string = describe(string)

    print list(describe_generator(1, 30))

    As requested, here's a more concise, but difficult to understand version
    of describe() using regexes:

    import re

    re_describe = re.compile(r"(.)\1*")
    def describe(string):
    runs = re_describe.finditer(str(string))

    return "".join("".join([str(len(run.group(0))), run.group(0)[0]])
    for run in runs)

    While it is fewer lines of code, it's not as easy to see what this
    *thing* does immediately upon looking at it. So I'd try to avoid this
    version, really.
    --
    Michael Hoffman
     
    Michael Hoffman, Aug 20, 2005
    #2
    1. Advertising

  3. Ray

    Guest

    Two versions of mine, one of the fastest (not using Psyco) and one of
    the shortest:

    .. from itertools import groupby
    ..
    .. def audioactiveFast(n):
    .. strl = {("1","1","1"): "31", ("1","1"): "21", ("1",): "11",
    .. ("2","2","2"): "32", ("2","2"): "22", ("2",): "12",
    .. ("3","3","3"): "33", ("3","3"): "23", ("3",): "13" }
    .. result = [1]
    .. prec = "1"
    .. for i in xrange(n-1):
    .. prec = "".join( strl[tuple(l)] for e,l in groupby(prec) )
    .. result.append( int(prec) )
    .. return result
    ..
    ..
    .. def audioactiveShort(n):
    .. s = [1]
    .. for i in xrange(n-1):
    .. r = 0
    .. for e,l in groupby(str(s[-1])):
    .. r = r*100 + len(list(l))*10 + int(e)
    .. s.append( r )
    .. return s

    Bye,
    bearophile
     
    , Aug 20, 2005
    #3
  4. Ray

    Ray Guest

    Damn, those are cool, man. Thanks! This Python thing keeps expanding
    and expanding my brain...

    Ray

    wrote:
    <snipped>
     
    Ray, Aug 20, 2005
    #4
  5. Ray

    Ray Guest

    Michael Hoffman wrote:
    > Ray wrote:
    >
    > > I just wrote a short script that generates look and say sequence. What
    > > do you Python guys think of it? Too "Java-ish"?

    >
    > Yes, but that's beside the point. :)


    Well... I'm always paranoid that I'm, you know, writing Java in Python
    :)

    <snipped>

    Thanks for the examples! That last one took me a while to understand. I
    like the way you approach it though (e.g.: going after clarity first
    instead of shortness).

    Cheers
    Ray
     
    Ray, Aug 20, 2005
    #5
  6. Ray wrote:

    > Well... I'm always paranoid that I'm, you know, writing Java in Python
    > :)


    The biggest mistakes I see are wrapping everything in a class
    unnecessarily, or using accessor methods instead of properties (ugh,
    ugh, ugh!). CamelCasingYourVariableNames is annoyingToMe but that's
    really a stylistic choice.

    > Thanks for the examples! That last one took me a while to understand. I
    > like the way you approach it though (e.g.: going after clarity first
    > instead of shortness).


    I think that is the Pythonic way. <wink> In general, I find conciseness
    in code is not worth all that much in its own right. Although sometimes
    it can make things easier to understand, which is good. There is a
    common belief that concise code = fast code, and it is easy to
    demonstrate that this is false. See, for example, this essay:
    <http://www.python.org/doc/essays/list2str.html>

    Also, if you haven't done so already, start up an interactive prompt and
    type "import this." Key to consider:

    Simple is better than complex.
    Complex is better than complicated.
    Sparse is better than dense.
    Readability counts.
    If the implementation is hard to explain, it's a bad idea.
    If the implementation is easy to explain, it may be a good idea.

    Best regards,
    --
    Michael Hoffman
     
    Michael Hoffman, Aug 20, 2005
    #6
  7. i guess, it is pythonchallenge.com level 10?
    if so, i used this thing:

    import re
    def enc(s):
    return ''.join('%s%s' % (len(a[0]),a[0][0]) for a in
    re.findall('((.)\\2*)', s))
     
    Christoph Rackwitz, Aug 20, 2005
    #7
  8. Ray

    Jeff Schwab Guest

    Christoph Rackwitz wrote:
    > i guess, it is pythonchallenge.com level 10?
    > if so, i used this thing:
    >
    > import re
    > def enc(s):
    > return ''.join('%s%s' % (len(a[0]),a[0][0]) for a in
    > re.findall('((.)\\2*)', s))
    >


    Don't do that!
     
    Jeff Schwab, Aug 20, 2005
    #8
  9. Why not? Because the regex isn't compiled?
    Don't tell me not to do something, tell me why i should'nt do it.
     
    Christoph Rackwitz, Aug 21, 2005
    #9
  10. Ray

    Ray Guest

    Christoph Rackwitz wrote:
    > Why not? Because the regex isn't compiled?
    > Don't tell me not to do something, tell me why i should'nt do it.


    No it's not the regex, it's because you just spoiled the challenge for
    everybody who hasn't solved level 10 yet...
     
    Ray, Aug 22, 2005
    #10
    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. Luigi Donatello Asero

    Please criticize my website.

    Luigi Donatello Asero, Sep 21, 2005, in forum: HTML
    Replies:
    336
    Views:
    7,545
    Luigi Donatello Asero
    Oct 23, 2005
  2. Luigi Donatello Asero

    Re:Please criticize my website.

    Luigi Donatello Asero, Sep 21, 2005, in forum: HTML
    Replies:
    8
    Views:
    500
    Luigi Donatello Asero
    Sep 21, 2005
  3. Luigi Donatello Asero

    Re: Please criticize my website.

    Luigi Donatello Asero, Sep 21, 2005, in forum: HTML
    Replies:
    72
    Views:
    1,970
  4. Luigi Donatello Asero

    Re: Please criticize my website.

    Luigi Donatello Asero, Sep 23, 2005, in forum: HTML
    Replies:
    25
    Views:
    938
    Neredbojias
    Sep 28, 2005
  5. Sergei Shelukhin

    criticize my code.. please?

    Sergei Shelukhin, Oct 16, 2004, in forum: Perl Misc
    Replies:
    3
    Views:
    154
    Ben Morrow
    Oct 18, 2004
Loading...

Share This Page