Skip to main content
added 45 characters in body
Source Link
janos
  • 113k
  • 15
  • 154
  • 396

I just need to know if it's decent or not.

Well, it's not.

  • You're not following PEP8, the official Python style guide
  • You have spaces mixed with tabs, which can lead to nasty bugs that will really hurt you

The code is buggy and strange in many places.


def on_message(self, message):
    self.modDirectory = 'modules/*.py'
    self.files = glob.glob(self.modDirectory)

The variable is called modDirectory, but its value is a glob. That's misleading. But the bigger problem is that modDirectory is an instance variable, but it's not used anywhere else in the posted code. This is better:

self.files = glob.glob('modules/*.py')

There is a bug here:

data = message.split(':', 1)
if len(data) == 1:
    cmd, args = data[0]

This assignment won't work, you'll get ValueError: too many values to unpack (expected 2).


What is going on here?

self.getCommand(cmd)
cmd = self.getCommand(key)

First you call self.getCommand and throw away the result? It's not clear what you're doing here and why. If the first call is necessary, that would mean that self.getCommand has side effects, which is a bad practice in general, especially for something that looks like a getter.


What's going on here:

if cmd:
    if cmd:

At this point I stopped reviewing. This is just too messy and far from decent. I bet you could spot these things yourself if you give it a careful read!

Here's what you can do to improve this, very easily:

  1. Install pep8 (pip install pep8). It's a command line tool that verifies specified file or an entire directory tree for PEP8 violations. As much as possible, follow it and fix everything it tells you.

  2. Install pyflakes (pip install pyflakes). Another command line tool, that checks for common mistakes in Python coding practices. Use it the same way as the pep8 tool.

If you do thisuse these tools to clean up your code and post again a new question, that will have a LOT better chance of being decent.!

I just need to know if it's decent or not.

Well, it's not.

  • You're not following PEP8, the official Python style guide
  • You have spaces mixed with tabs, which can lead to nasty bugs that will really hurt you

The code is buggy and strange in many places.


def on_message(self, message):
    self.modDirectory = 'modules/*.py'
    self.files = glob.glob(self.modDirectory)

The variable is called modDirectory, but its value is a glob. That's misleading. But the bigger problem is that modDirectory is an instance variable, but it's not used anywhere else in the posted code. This is better:

self.files = glob.glob('modules/*.py')

There is a bug here:

data = message.split(':', 1)
if len(data) == 1:
    cmd, args = data[0]

This assignment won't work, you'll get ValueError: too many values to unpack (expected 2).


What is going on here?

self.getCommand(cmd)
cmd = self.getCommand(key)

First you call self.getCommand and throw away the result? It's not clear what you're doing here and why. If the first call is necessary, that would mean that self.getCommand has side effects, which is a bad practice in general, especially for something that looks like a getter.


What's going on here:

if cmd:
    if cmd:

At this point I stopped reviewing. This is just too messy and far from decent. I bet you could spot these things yourself if you give it a careful read!

Here's what you can do to improve this, very easily:

  1. Install pep8 (pip install pep8). It's a command line tool that verifies specified file or an entire directory tree for PEP8 violations. As much as possible, follow it and fix everything it tells you.

  2. Install pyflakes (pip install pyflakes). Another command line tool, that checks for common mistakes in Python coding practices. Use it the same way as the pep8 tool.

If you do this and post again, that will have a LOT better chance of being decent.

I just need to know if it's decent or not.

Well, it's not.

  • You're not following PEP8, the official Python style guide
  • You have spaces mixed with tabs, which can lead to nasty bugs that will really hurt you

The code is buggy and strange in many places.


def on_message(self, message):
    self.modDirectory = 'modules/*.py'
    self.files = glob.glob(self.modDirectory)

The variable is called modDirectory, but its value is a glob. That's misleading. But the bigger problem is that modDirectory is an instance variable, but it's not used anywhere else in the posted code. This is better:

self.files = glob.glob('modules/*.py')

There is a bug here:

data = message.split(':', 1)
if len(data) == 1:
    cmd, args = data[0]

This assignment won't work, you'll get ValueError: too many values to unpack (expected 2).


What is going on here?

self.getCommand(cmd)
cmd = self.getCommand(key)

First you call self.getCommand and throw away the result? It's not clear what you're doing here and why. If the first call is necessary, that would mean that self.getCommand has side effects, which is a bad practice in general, especially for something that looks like a getter.


What's going on here:

if cmd:
    if cmd:

At this point I stopped reviewing. This is just too messy and far from decent. I bet you could spot these things yourself if you give it a careful read!

Here's what you can do to improve this, very easily:

  1. Install pep8 (pip install pep8). It's a command line tool that verifies specified file or an entire directory tree for PEP8 violations. As much as possible, follow it and fix everything it tells you.

  2. Install pyflakes (pip install pyflakes). Another command line tool, that checks for common mistakes in Python coding practices. Use it the same way as the pep8 tool.

If you use these tools to clean up your code and post again a new question, that will have a LOT better chance of being decent!

Source Link
janos
  • 113k
  • 15
  • 154
  • 396

I just need to know if it's decent or not.

Well, it's not.

  • You're not following PEP8, the official Python style guide
  • You have spaces mixed with tabs, which can lead to nasty bugs that will really hurt you

The code is buggy and strange in many places.


def on_message(self, message):
    self.modDirectory = 'modules/*.py'
    self.files = glob.glob(self.modDirectory)

The variable is called modDirectory, but its value is a glob. That's misleading. But the bigger problem is that modDirectory is an instance variable, but it's not used anywhere else in the posted code. This is better:

self.files = glob.glob('modules/*.py')

There is a bug here:

data = message.split(':', 1)
if len(data) == 1:
    cmd, args = data[0]

This assignment won't work, you'll get ValueError: too many values to unpack (expected 2).


What is going on here?

self.getCommand(cmd)
cmd = self.getCommand(key)

First you call self.getCommand and throw away the result? It's not clear what you're doing here and why. If the first call is necessary, that would mean that self.getCommand has side effects, which is a bad practice in general, especially for something that looks like a getter.


What's going on here:

if cmd:
    if cmd:

At this point I stopped reviewing. This is just too messy and far from decent. I bet you could spot these things yourself if you give it a careful read!

Here's what you can do to improve this, very easily:

  1. Install pep8 (pip install pep8). It's a command line tool that verifies specified file or an entire directory tree for PEP8 violations. As much as possible, follow it and fix everything it tells you.

  2. Install pyflakes (pip install pyflakes). Another command line tool, that checks for common mistakes in Python coding practices. Use it the same way as the pep8 tool.

If you do this and post again, that will have a LOT better chance of being decent.