Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

Here are a few suggestions:

String comparisons should be done with == (and !=) instead of is (and is not). is works, but implies you are comparing identity (which is usually a memory address), whereas == is comparing value. See http://stackoverflow.com/a/2988117/331473https://stackoverflow.com/a/2988117/331473 for more information.


Your boolean configs (eg: minify_html) should be actual boolean values True/False instead of 1/0. Also, when you are checking for these, you should drop the comparison. Example:

if minify_html == 1:     # or minify_html == True 
   ...                   #    (if you've converted these to booleans)

can be written as:

if minify_html:
    ...

Using module level vars as configs is generally ok for a few things, but once you've got a whole catalog of things, it can be a bit much to keep track of. Several times while reviewing your code I found myself saying "Where did that var come from?".

If you want to address this, you could put these in a dictionary so it's "namespaced" so to speak. Example:

config = {
    "site_url": "...",
    "site_name": "..."
}

then in your code, you can more easily spot config bits:

if config['minify_html']:
    ....

There's more cleanup that can be done, but these are just the first few things that popped out at me. One more thing... I don't have time address it now, but the long chain of if/elses in buildhtmlheader could be refactored a bit to make things a little less redundant.

Here are a few suggestions:

String comparisons should be done with == (and !=) instead of is (and is not). is works, but implies you are comparing identity (which is usually a memory address), whereas == is comparing value. See http://stackoverflow.com/a/2988117/331473 for more information.


Your boolean configs (eg: minify_html) should be actual boolean values True/False instead of 1/0. Also, when you are checking for these, you should drop the comparison. Example:

if minify_html == 1:     # or minify_html == True 
   ...                   #    (if you've converted these to booleans)

can be written as:

if minify_html:
    ...

Using module level vars as configs is generally ok for a few things, but once you've got a whole catalog of things, it can be a bit much to keep track of. Several times while reviewing your code I found myself saying "Where did that var come from?".

If you want to address this, you could put these in a dictionary so it's "namespaced" so to speak. Example:

config = {
    "site_url": "...",
    "site_name": "..."
}

then in your code, you can more easily spot config bits:

if config['minify_html']:
    ....

There's more cleanup that can be done, but these are just the first few things that popped out at me. One more thing... I don't have time address it now, but the long chain of if/elses in buildhtmlheader could be refactored a bit to make things a little less redundant.

Here are a few suggestions:

String comparisons should be done with == (and !=) instead of is (and is not). is works, but implies you are comparing identity (which is usually a memory address), whereas == is comparing value. See https://stackoverflow.com/a/2988117/331473 for more information.


Your boolean configs (eg: minify_html) should be actual boolean values True/False instead of 1/0. Also, when you are checking for these, you should drop the comparison. Example:

if minify_html == 1:     # or minify_html == True 
   ...                   #    (if you've converted these to booleans)

can be written as:

if minify_html:
    ...

Using module level vars as configs is generally ok for a few things, but once you've got a whole catalog of things, it can be a bit much to keep track of. Several times while reviewing your code I found myself saying "Where did that var come from?".

If you want to address this, you could put these in a dictionary so it's "namespaced" so to speak. Example:

config = {
    "site_url": "...",
    "site_name": "..."
}

then in your code, you can more easily spot config bits:

if config['minify_html']:
    ....

There's more cleanup that can be done, but these are just the first few things that popped out at me. One more thing... I don't have time address it now, but the long chain of if/elses in buildhtmlheader could be refactored a bit to make things a little less redundant.

Source Link
Adam Wagner
  • 343
  • 1
  • 10

Here are a few suggestions:

String comparisons should be done with == (and !=) instead of is (and is not). is works, but implies you are comparing identity (which is usually a memory address), whereas == is comparing value. See http://stackoverflow.com/a/2988117/331473 for more information.


Your boolean configs (eg: minify_html) should be actual boolean values True/False instead of 1/0. Also, when you are checking for these, you should drop the comparison. Example:

if minify_html == 1:     # or minify_html == True 
   ...                   #    (if you've converted these to booleans)

can be written as:

if minify_html:
    ...

Using module level vars as configs is generally ok for a few things, but once you've got a whole catalog of things, it can be a bit much to keep track of. Several times while reviewing your code I found myself saying "Where did that var come from?".

If you want to address this, you could put these in a dictionary so it's "namespaced" so to speak. Example:

config = {
    "site_url": "...",
    "site_name": "..."
}

then in your code, you can more easily spot config bits:

if config['minify_html']:
    ....

There's more cleanup that can be done, but these are just the first few things that popped out at me. One more thing... I don't have time address it now, but the long chain of if/elses in buildhtmlheader could be refactored a bit to make things a little less redundant.