code review

Discussion in 'Python' started by Littlefield, Tyler, Jun 29, 2012.

  1. Hello all:
    I was curious if someone wouldn't mind poking at some code.
    I have an idea for a game I want to write (and if this works I want to
    use this as a framework for another project), but I'd like to make sure
    I'm doing things correctly/there's not a better way to do things. My
    concern is I'm going to get way far into this, then realize I totally
    broke something. So, if someone wouldn't mind taking a peek I'd
    appreciate it. My concerns are:
    1) style/cleanlyness: does everything look ok?
    2) Workability: is there a better way to do what is there?
    3) Speed: am I doing something that could be improved? I don't want to
    spend a ton of time looking for non-existent bottlenecks and trying to
    improve on them, but if I'm doing something that's bad, I'd like to fix it.

    The project page is at:
    http://code.google.com/p/pymud
    Any information is greatly appreciated.

    --
    Take care,
    Ty
    http://tds-solutions.net
    The aspen project: a barebones light-weight mud engine:
    http://code.google.com/p/aspenmud
    He that will not reason is a bigot; he that cannot reason is a fool; he that dares not reason is a slave.
     
    Littlefield, Tyler, Jun 29, 2012
    #1
    1. Advertising

  2. Littlefield, Tyler

    alex23 Guest

    On Jun 29, 12:57 pm, "Littlefield, Tyler" <> wrote:
    > I was curious if someone wouldn't mind poking at some code.
    > The project page is at:http://code.google.com/p/pymud
    > Any information is greatly appreciated.


    I couldn't find any actual code at that site, the git repository is
    currently empty.
     
    alex23, Jun 29, 2012
    #2
    1. Advertising

  3. On Thu, 28 Jun 2012 20:58:15 -0700, alex23 wrote:

    > On Jun 29, 12:57 pm, "Littlefield, Tyler" <> wrote:
    >> I was curious if someone wouldn't mind poking at some code. The project
    >> page is at:http://code.google.com/p/pymud Any information is greatly
    >> appreciated.

    >
    > I couldn't find any actual code at that site, the git repository is
    > currently empty.


    Given that all code contains bugs, that's the best sort of repository!



    --
    Steven
     
    Steven D'Aprano, Jun 29, 2012
    #3
  4. On Fri, Jun 29, 2012 at 5:31 PM, Steven D'Aprano
    <> wrote:
    > Given that all code contains bugs, that's the best sort of repository!


    Only in the sense that a cheese shop can be lauded for its cleanliness...

    But I am somewhat curious to see the OP's actual code. MUDs are my
    personal specialty.

    ChrisA
     
    Chris Angelico, Jun 29, 2012
    #4
  5. On 6/29/2012 1:31 AM, Steven D'Aprano wrote:
    > On Thu, 28 Jun 2012 20:58:15 -0700, alex23 wrote:
    >
    >> On Jun 29, 12:57 pm, "Littlefield, Tyler" <> wrote:
    >>> I was curious if someone wouldn't mind poking at some code. The project
    >>> page is at:http://code.google.com/p/pymud Any information is greatly
    >>> appreciated.

    >> I couldn't find any actual code at that site, the git repository is
    >> currently empty.


    OOPS, sorry. Apparently I'm not as good with git as I thought.
    Everything's in the repo now.
     
    Littlefield, Tyler, Jun 29, 2012
    #5
  6. Littlefield, Tyler

    Alister Guest

    On Fri, 29 Jun 2012 09:03:22 -0600, Littlefield, Tyler wrote:

    > On 6/29/2012 1:31 AM, Steven D'Aprano wrote:
    >> On Thu, 28 Jun 2012 20:58:15 -0700, alex23 wrote:
    >>
    >>> On Jun 29, 12:57 pm, "Littlefield, Tyler" <> wrote:
    >>>> I was curious if someone wouldn't mind poking at some code. The
    >>>> project page is at:http://code.google.com/p/pymud Any information is
    >>>> greatly appreciated.
    >>> I couldn't find any actual code at that site, the git repository is
    >>> currently empty.

    >
    > OOPS, sorry. Apparently I'm not as good with git as I thought.
    > Everything's in the repo now.


    I am no expert but from what have picked up so far

    from x import

    is frowned upon in most cases

    also this section in main strikes me as a bit odd and convoluted

    w = world()
    serv = server(client)
    w.server = serv
    serv.world = w

    I think you are cross referencing classes & would be better to
    investigate inheritance.


    --
    The bogosity meter just pegged.
     
    Alister, Jun 29, 2012
    #6
  7. Littlefield, Tyler

    MRAB Guest

    On 29/06/2012 20:41, Alister wrote:
    > On Fri, 29 Jun 2012 09:03:22 -0600, Littlefield, Tyler wrote:
    >
    >> On 6/29/2012 1:31 AM, Steven D'Aprano wrote:
    >>> On Thu, 28 Jun 2012 20:58:15 -0700, alex23 wrote:
    >>>
    >>>> On Jun 29, 12:57 pm, "Littlefield, Tyler" <> wrote:
    >>>>> I was curious if someone wouldn't mind poking at some code. The
    >>>>> project page is at:http://code.google.com/p/pymud Any information is
    >>>>> greatly appreciated.
    >>>> I couldn't find any actual code at that site, the git repository is
    >>>> currently empty.

    >>
    >> OOPS, sorry. Apparently I'm not as good with git as I thought.
    >> Everything's in the repo now.

    >
    > I am no expert but from what have picked up so far
    >
    > from x import
    >
    > is frowned upon in most cases
    >

    I think you mean:

    from x import *

    > also this section in main strikes me as a bit odd and convoluted
    >
    > w = world()
    > serv = server(client)
    > w.server = serv
    > serv.world = w
    >
    > I think you are cross referencing classes & would be better to
    > investigate inheritance.
    >
    >
     
    MRAB, Jun 29, 2012
    #7
  8. On Friday, 29 June 2012 20:41:11 UTC+1, Alister wrote:
    > On Fri, 29 Jun 2012 09:03:22 -0600, Littlefield, Tyler wrote:
    >
    > > On 6/29/2012 1:31 AM, Steven D'Aprano wrote:
    > >> On Thu, 28 Jun 2012 20:58:15 -0700, alex23 wrote:
    > >>
    > >>> On Jun 29, 12:57 pm, "Littlefield, Tyler" <> wrote:
    > >>>> I was curious if someone wouldn't mind poking at some code. The
    > >>>> project page is at:http://code.google.com/p/pymud Any information is
    > >>>> greatly appreciated.
    > >>> I couldn't find any actual code at that site, the git repository is
    > >>> currently empty.

    > >
    > > OOPS, sorry. Apparently I'm not as good with git as I thought.
    > > Everything's in the repo now.

    >
    > I am no expert but from what have picked up so far
    >
    > from x import
    >
    > is frowned upon in most cases


    from x import * is frowned upon, however, from x import y is fine IMHO.
    >
    > also this section in main strikes me as a bit odd and convoluted
    >
    > w = world()
    > serv = server(client)
    > w.server = serv
    > serv.world = w
    >
    > I think you are cross referencing classes & would be better to
    > investigate inheritance.
    >


    Generally speaking, read PEP8 and apply it please, there are tools like pylint that can help you with that. It also seems you are doing things quite java like, but I guess that is just a thing of getting used to python.

    If you are planning to let your code being used like a framework that is extended by others, try to avoid more advanced functions just because they seem handy, always ask yourself is it clearer?

    Try to unit-test your code and try to gain some decent code coverage, that will increase maturity of your code rather quickly.

    But for the rest it looks like you are good in organizing it all in sub-modules, which is a very nice thing to see.

    Good luck!

    --
    mph
     
    Martin P. Hellwig, Jun 29, 2012
    #8
  9. Littlefield, Tyler

    Alister Guest

    On Fri, 29 Jun 2012 13:27:54 -0700, Martin P. Hellwig wrote:

    > On Friday, 29 June 2012 20:41:11 UTC+1, Alister wrote:
    >> On Fri, 29 Jun 2012 09:03:22 -0600, Littlefield, Tyler wrote:
    >>
    >> > On 6/29/2012 1:31 AM, Steven D'Aprano wrote:
    >> >> On Thu, 28 Jun 2012 20:58:15 -0700, alex23 wrote:
    >> >>
    >> >>> On Jun 29, 12:57 pm, "Littlefield, Tyler" <>
    >> >>> wrote:
    >> >>>> I was curious if someone wouldn't mind poking at some code. The
    >> >>>> project page is at:http://code.google.com/p/pymud Any information
    >> >>>> is greatly appreciated.
    >> >>> I couldn't find any actual code at that site, the git repository is
    >> >>> currently empty.
    >> >
    >> > OOPS, sorry. Apparently I'm not as good with git as I thought.
    >> > Everything's in the repo now.

    >>
    >> I am no expert but from what have picked up so far
    >>
    >> from x import
    >>
    >> is frowned upon in most cases

    >
    > from x import * is frowned upon, however, from x import y is fine IMHO.
    >>

    well I said I was no expert & picking things up. re investigation I see
    your reasoning and yes it was the from X import * I was thinking of.

    Although a simple import X retaining the name-space ref does make it easy
    to identify the origins of a function (at the expense of more typing)
    --
    Flying is the second greatest feeling you can have. The greatest feeling?
    Landing... Landing is the greatest feeling you can have.
     
    Alister, Jun 29, 2012
    #9
  10. I am no expert but from what have picked up so far from x import is
    frowned upon in most cases also this section in main strikes me as a bit
    odd and convoluted w = world() serv = server(client) w.server = serv
    serv.world = w I think you are cross referencing classes & would be
    better to investigate inheritance.

    From what I understand and how I've always employed it, inheritance is
    ment when you wish to give a class characteristics of another class. All
    I'm doing here is setting the world and server classes on each other, so
    they can call one another. This probably isn't needed in case of
    serv.server = w, but for sure the other way around.

    --
    Take care,
    Ty
    http://tds-solutions.net
    The aspen project: a barebones light-weight mud engine:
    http://code.google.com/p/aspenmud
    He that will not reason is a bigot; he that cannot reason is a fool; he that dares not reason is a slave.
     
    Littlefield, Tyler, Jun 29, 2012
    #10
  11. On Fri, 29 Jun 2012 20:43:02 GMT, Alister <>
    declaimed the following in gmane.comp.python.general:

    > Although a simple import X retaining the name-space ref does make it easy
    > to identify the origins of a function (at the expense of more typing)


    Well, there is the middle point...

    import some_long_module_name as slmn

    which retains the individual namespace, but reduces the amount of
    typing.
    --
    Wulfraed Dennis Lee Bieber AF6VN
    HTTP://wlfraed.home.netcom.com/
     
    Dennis Lee Bieber, Jun 30, 2012
    #11
  12. On Fri, 29 Jun 2012 19:41:11 +0000, Alister wrote:

    > also this section in main strikes me as a bit odd and convoluted
    >
    > w = world()
    > serv = server(client)
    > w.server = serv
    > serv.world = w
    >
    > I think you are cross referencing classes & would be better to
    > investigate inheritance.


    What you show above is composition, and is a perfectly valid technique,
    and in fact should often be *preferred* to inheritance.

    The only slightly dubious part, and I stress *slightly*, is that there is
    a circular reference: w refers to serv, and serv refers back to w. While
    legitimate, it is a very slight "code smell" that should be investigated.
    If there is a way to get the same result without the circular reference,
    that would be preferred.

    For example, perhaps server methods that need to know the world could
    take it as an explicit argument, rather than fetching it implicitly from
    server.world.

    Or, a moderately advanced technique, use a weak-ref instead.

    Inheritance should only be used to model "is-a" relationships. For
    example, Herbie the Love Bug is a Volkswagen Beetle, so inheritance is
    appropriate.

    http://en.wikipedia.org/wiki/Herbie


    class Vehicle(object):
    pass

    class Car(Vehicle):
    pass

    class Beetle(Car):
    pass

    herbie = Beetle()

    Composition should be used to model "has-a" relationships. For example, a
    car has an engine, it is not a kind of engine, and so inheritance is
    inappropriate and composition should be used. I would re-write the Car
    class as follows:

    class Engine(object):
    pass

    class Car(Vehicle):
    def __init__(self):
    self.engine = Engine()

    So now we can talk about Herbie's engine:

    herbie.engine # Herbie, being a car, has an engine, he is not an engine



    --
    Steven
     
    Steven D'Aprano, Jun 30, 2012
    #12
  13. Littlefield, Tyler

    Terry Reedy Guest

    On 6/29/2012 4:49 PM, Littlefield, Tyler wrote:
    > I am no expert but from what have picked up so far from x import is
    > frowned upon in most cases


    from x import *
    # frowned on by many as reader will not necessarily know what that
    imports, conflicts are possible, and if you import * twice, reader may
    really not know what came from where

    from x import y,x # fine, common, used in stdlib
    import x as y # ditto
    from x import y as z # ditto
    --
    Terry Jan Reedy
     
    Terry Reedy, Jun 30, 2012
    #13
  14. Littlefield, Tyler

    Terry Reedy Guest

    On 6/29/2012 4:43 PM, Alister wrote:

    >> from x import * is frowned upon, however, from x import y is fine IMHO.
    >>>

    > well I said I was no expert & picking things up. re investigation I see
    > your reasoning and yes it was the from X import * I was thinking of.
    >
    > Although a simple import X retaining the name-space ref does make it easy
    > to identify the origins of a function (at the expense of more typing)


    import tkinter as tk
    for example, to minimize extra typing while identifying source.
    ___
    Terry Jan Reedy
     
    Terry Reedy, Jun 30, 2012
    #14
  15. Littlefield, Tyler

    Alister Guest

    On Sat, 30 Jun 2012 02:28:52 +0000, Steven D'Aprano wrote:

    > On Fri, 29 Jun 2012 19:41:11 +0000, Alister wrote:
    >
    >> also this section in main strikes me as a bit odd and convoluted
    >>
    >> w = world()
    >> serv = server(client)
    >> w.server = serv serv.world = w
    >>
    >> I think you are cross referencing classes & would be better to
    >> investigate inheritance.

    >
    > What you show above is composition, and is a perfectly valid technique,
    > and in fact should often be *preferred* to inheritance.
    >
    > The only slightly dubious part, and I stress *slightly*, is that there
    > is a circular reference: w refers to serv, and serv refers back to w.
    > While legitimate, it is a very slight "code smell" that should be
    > investigated.
    > If there is a way to get the same result without the circular reference,
    > that would be preferred.
    >
    > For example, perhaps server methods that need to know the world could
    > take it as an explicit argument, rather than fetching it implicitly from
    > server.world.
    >
    > Or, a moderately advanced technique, use a weak-ref instead.
    >
    > Inheritance should only be used to model "is-a" relationships. For
    > example, Herbie the Love Bug is a Volkswagen Beetle, so inheritance is
    > appropriate.
    >
    > http://en.wikipedia.org/wiki/Herbie
    >
    >
    > class Vehicle(object):
    > pass
    >
    > class Car(Vehicle):
    > pass
    >
    > class Beetle(Car):
    > pass
    >
    > herbie = Beetle()
    >
    > Composition should be used to model "has-a" relationships. For example,
    > a car has an engine, it is not a kind of engine, and so inheritance is
    > inappropriate and composition should be used. I would re-write the Car
    > class as follows:
    >
    > class Engine(object):
    > pass
    >
    > class Car(Vehicle):
    > def __init__(self):
    > self.engine = Engine()
    >
    > So now we can talk about Herbie's engine:
    >
    > herbie.engine # Herbie, being a car, has an engine, he is not an engine


    I probably was not to clear (due to my own inexperience) it was the
    circular references that grabbed my attention, it may be OK but suggests
    to me there may be a better approach.




    --
    ((lambda (foo) (bar foo)) (baz))
     
    Alister, Jun 30, 2012
    #15
  16. Littlefield, Tyler

    Alister Guest

    On Fri, 29 Jun 2012 14:49:11 -0600, Littlefield, Tyler wrote:

    > I am no expert but from what have picked up so far from x import is
    > frowned upon in most cases also this section in main strikes me as a bit
    > odd and convoluted w = world() serv = server(client) w.server = serv
    > serv.world = w I think you are cross referencing classes & would be
    > better to investigate inheritance.
    >
    > From what I understand and how I've always employed it, inheritance is
    > ment when you wish to give a class characteristics of another class. All
    > I'm doing here is setting the world and server classes on each other, so
    > they can call one another. This probably isn't needed in case of
    > serv.server = w, but for sure the other way around.


    I was not too sure of exactly why the code looked odd.
    as mentioned in another post I should really have referred to the
    circular references.

    I am new to python (about 6 months of home hacking), I looked at the code
    to see if it could improve my knowledge & my responses have been intended
    to spark a 2 way discussion of the pro's & cons of the approach.
    So far that seems to be working, I expect by the end of this I will have
    learnt much about real world python apps.







    --
    Nobody ever died from oven crude poisoning.
     
    Alister, Jun 30, 2012
    #16
  17. Littlefield, Tyler

    Alister Guest

    On Sat, 30 Jun 2012 09:31:53 +0000, Alister wrote:

    > On Fri, 29 Jun 2012 14:49:11 -0600, Littlefield, Tyler wrote:
    >
    >> I am no expert but from what have picked up so far from x import is
    >> frowned upon in most cases also this section in main strikes me as a
    >> bit odd and convoluted w = world() serv = server(client) w.server =
    >> serv serv.world = w I think you are cross referencing classes & would
    >> be better to investigate inheritance.
    >>
    >> From what I understand and how I've always employed it, inheritance is
    >> ment when you wish to give a class characteristics of another class.
    >> All I'm doing here is setting the world and server classes on each
    >> other, so they can call one another. This probably isn't needed in case
    >> of serv.server = w, but for sure the other way around.

    >
    > I was not too sure of exactly why the code looked odd.
    > as mentioned in another post I should really have referred to the
    > circular references.
    >
    > I am new to python (about 6 months of home hacking), I looked at the
    > code to see if it could improve my knowledge & my responses have been
    > intended to spark a 2 way discussion of the pro's & cons of the
    > approach.
    > So far that seems to be working, I expect by the end of this I will have
    > learnt much about real world python apps.


    perhaps now is a good time for me to look at the rest of the modules to
    see if i can work out exactly what these circular references do.
    btw where or what is pants.py?



    --
    If the path be beautiful, let us not ask where it leads.
    -- Anatole France
     
    Alister, Jun 30, 2012
    #17
  18. Littlefield, Tyler

    Alister Guest

    On Fri, 29 Jun 2012 09:03:22 -0600, Littlefield, Tyler wrote:

    > On 6/29/2012 1:31 AM, Steven D'Aprano wrote:
    >> On Thu, 28 Jun 2012 20:58:15 -0700, alex23 wrote:
    >>
    >>> On Jun 29, 12:57 pm, "Littlefield, Tyler" <> wrote:
    >>>> I was curious if someone wouldn't mind poking at some code. The
    >>>> project page is at:http://code.google.com/p/pymud Any information is
    >>>> greatly appreciated.
    >>> I couldn't find any actual code at that site, the git repository is
    >>> currently empty.

    >
    > OOPS, sorry. Apparently I'm not as good with git as I thought.
    > Everything's in the repo now.


    I think I may be on firmer grounds with the next few:

    isValidPassword can be simplified to

    def isValidPassword(password:
    count=len(password)
    return count>= mud.minpass and count<= mud.maxpass

    ( I used count to save finding the length of password twice although it
    probably makes no difference in this scenario)

    similar construct can be used for isValidUser

    def isValidUser(name):
    if name.isalpha():
    count=len(name)
    return count>=mud.minname and count >mud.maxname
    return False


    --
    No one wants war.
    -- Kirk, "Errand of Mercy", stardate 3201.7
     
    Alister, Jun 30, 2012
    #18
  19. Littlefield, Tyler

    Peter Otten Guest

    Alister wrote:

    > I think I may be on firmer grounds with the next few:
    >
    > isValidPassword can be simplified to
    >
    > def isValidPassword(password:
    > count=len(password)
    > return count>= mud.minpass and count<= mud.maxpass
    >
    > ( I used count to save finding the length of password twice although it
    > probably makes no difference in this scenario)


    If you spell it

    def is_valid_password(password):
    return mud.minpass <= len(password) <= mud.maxpass

    it is even easier to see that you are performing an interval check.
     
    Peter Otten, Jun 30, 2012
    #19
  20. Peter Otten wrote:

    > If you spell it
    >
    > def is_valid_password(password):
    > return mud.minpass <= len(password) <= mud.maxpass
    >
    > it is even easier to see that you are performing an interval check.


    This is probably a tautology around here, but *what* *a* *great*
    *programming* *language*.

    --
    PointedEars

    Please do not Cc: me. / Bitte keine Kopien per E-Mail.
     
    Thomas 'PointedEars' Lahn, Jun 30, 2012
    #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. Volodymyr Sadovyy

    Code write \ code review productivity

    Volodymyr Sadovyy, Apr 23, 2004, in forum: Java
    Replies:
    8
    Views:
    770
    Roedy Green
    Apr 25, 2004
  2. Otto Wyss
    Replies:
    5
    Views:
    435
    Robert Vazan
    Sep 7, 2003
  3. andrew blah
    Replies:
    6
    Views:
    364
    andrew blah
    Oct 17, 2004
  4. Josiah Carlson
    Replies:
    1
    Views:
    357
    Andrew Clover
    Oct 13, 2004
  5. www
    Replies:
    51
    Views:
    1,501
Loading...

Share This Page