Skip to main content
edited body
Source Link
S.Lott
  • 750
  • 3
  • 6
  1. Your indentation is wrong in the posted code. Please edit your question to (a) paste the code then (b) select all the code and (c) click the {} button to get it all indented correctly.

  2. A single large class which does everything is a poor design. Each individual "command" should be a separate class.

  3. Your overall command-line processing loop (SQAST.cmdloop()) should be a separate class which relies on instances of the other command classes. Each of the various do_this() methods would then create an instance of one of the other classes, and then execute a method of that other class.

Yes, it seems like a lot of extra classes. However, breaking the individual commands into separate classes allows you to reuse the commands in other applications without copying and pasting code. Also, making separate classes for each command makes the shared information among the commands absolutely explicit.

  1. The stuff done at the class level inside the SQAST class (and outside any of the method functions) is often a Very Bad Idea. Use __init__ for this kind of thing.

  2. Try to avoid using "filename.endswith('.txt')"filename.endswith('.txt'). It's slightly better to use os.splitext to split the name and the extension and then compare the results. base, ext = os.path.splitext(filename). Yes, it's an extra line of code, but it makes your script slightly more portable to operating systems that might have peculiar file name rules (like Open VMS, for example).

  1. Your indentation is wrong in the posted code. Please edit your question to (a) paste the code then (b) select all the code and (c) click the {} button to get it all indented correctly.

  2. A single large class which does everything is a poor design. Each individual "command" should be a separate class.

  3. Your overall command-line processing loop (SQAST.cmdloop()) should be a separate class which relies on instances of the other command classes. Each of the various do_this() methods would then create an instance of one of the other classes, and then execute a method of that other class.

Yes, it seems like a lot of extra classes. However, breaking the individual commands into separate classes allows you to reuse the commands in other applications without copying and pasting code. Also, making separate classes for each command makes the shared information among the commands absolutely explicit.

  1. The stuff done at the class level inside the SQAST class (and outside any of the method functions) is often a Very Bad Idea. Use __init__ for this kind of thing.

  2. Try to avoid using "filename.endswith('.txt')". It's slightly better to use os.splitext to split the name and the extension and then compare the results. base, ext = os.path.splitext(filename). Yes, it's an extra line of code, but it makes your script slightly more portable to operating systems that might have peculiar file name rules (like Open VMS, for example).

  1. Your indentation is wrong in the posted code. Please edit your question to (a) paste the code then (b) select all the code and (c) click the {} button to get it all indented correctly.

  2. A single large class which does everything is a poor design. Each individual "command" should be a separate class.

  3. Your overall command-line processing loop (SQAST.cmdloop()) should be a separate class which relies on instances of the other command classes. Each of the various do_this() methods would then create an instance of one of the other classes, and then execute a method of that other class.

Yes, it seems like a lot of extra classes. However, breaking the individual commands into separate classes allows you to reuse the commands in other applications without copying and pasting code. Also, making separate classes for each command makes the shared information among the commands absolutely explicit.

  1. The stuff done at the class level inside the SQAST class (and outside any of the method functions) is often a Very Bad Idea. Use __init__ for this kind of thing.

  2. Try to avoid using filename.endswith('.txt'). It's slightly better to use os.splitext to split the name and the extension and then compare the results. base, ext = os.path.splitext(filename). Yes, it's an extra line of code, but it makes your script slightly more portable to operating systems that might have peculiar file name rules (like Open VMS, for example).

Source Link
S.Lott
  • 750
  • 3
  • 6

  1. Your indentation is wrong in the posted code. Please edit your question to (a) paste the code then (b) select all the code and (c) click the {} button to get it all indented correctly.

  2. A single large class which does everything is a poor design. Each individual "command" should be a separate class.

  3. Your overall command-line processing loop (SQAST.cmdloop()) should be a separate class which relies on instances of the other command classes. Each of the various do_this() methods would then create an instance of one of the other classes, and then execute a method of that other class.

Yes, it seems like a lot of extra classes. However, breaking the individual commands into separate classes allows you to reuse the commands in other applications without copying and pasting code. Also, making separate classes for each command makes the shared information among the commands absolutely explicit.

  1. The stuff done at the class level inside the SQAST class (and outside any of the method functions) is often a Very Bad Idea. Use __init__ for this kind of thing.

  2. Try to avoid using "filename.endswith('.txt')". It's slightly better to use os.splitext to split the name and the extension and then compare the results. base, ext = os.path.splitext(filename). Yes, it's an extra line of code, but it makes your script slightly more portable to operating systems that might have peculiar file name rules (like Open VMS, for example).