7
\$\begingroup\$

I have two folders query and subject. The following function sorts alphabetically and provides the query and subject as a separate list. The file names are mixed of numbers and I found the sort function works perfectly well. Any comments to improve?

import os
import re


subject_path = "/Users/catuf/Desktop/subject_fastafiles/"
query_path = "/Users/catuf/Desktop/query_fastafiles"



def sorted_nicely(l):

    """ Sort the given iterable in the way that humans expect. https://blog.codinghorror.com/sorting-for-humans-natural-sort-order/ """
    convert = lambda text: int(text) if text.isdigit() else text
    alphanum_key = lambda key: [ convert(c) for c in re.split('([0-9]+)', key) ]
    return sorted(l, key = alphanum_key)


def subject_list_fastafiles():

    subject_fastafiles = sorted_nicely([fastafile for fastafile in os.listdir(subject_path) if os.path.isfile(os.path.join(subject_path, fastafile))])

    return subject_fastafiles

def query_list_fastafiles():

    query_fastafiles = sorted_nicely([fastafile for fastafile in os.listdir(query_path) if os.path.isfile(os.path.join(query_path, fastafile))])

    return query_fastafiles

def filter_files_ending_with_one(sorted_files):

    """ The function filters the files end with 1 """

    files_end_with_one = [name for name in subject_fastafiles if name[-1].isdigit() and not name[-2].isdigit() == 1]

    return files_end_with_one


subject_fastafiles = subject_list_fastafiles()

query_fastafiles = query_list_fastafiles()

subject_files_ending_with_one = filter_files_ending_with_one(subject_fastafiles)
\$\endgroup\$
2
  • 2
    \$\begingroup\$ I rolled back to Revision 4, as your most recent edit invalidates current answers. \$\endgroup\$
    – Linny
    Commented Sep 12, 2019 at 14:09
  • \$\begingroup\$ From yesterday comments I was trying to name properly. I forgot to name properly. I think I have edited properly now. \$\endgroup\$
    – catuf
    Commented Sep 12, 2019 at 14:09

4 Answers 4

6
\$\begingroup\$

Docstrings

You should include a docstring at the beginning of every function, class, and module you write. This will allow documentation to identify what your code is supposed to do. This also helps other readers understand how your code works. I see that you already have a couple for your functions, but stay consistent.

Parameter Names

Parameter names should be descriptive enough to be able to tell what should be passed. While l might be obvious to some programmers as an iterable, to others it might not. Since you're passing a list, renaming it to list_ (to avoid using reserved word list) makes it more obvious what you're passing, and accepting.

Constant Variables

When you have a constant in your program, it should be UPPER_CASE to identify it as such.

Code Reduction

You want as little code as possible in your program. So, instead of:

def subject_list_fastafiles():
    """ Method Docstring """
    subject_fastafiles = sorted_nicely([fastafile for fastafile in os.listdir(subject_path) if os.path.isfile(os.path.join(subject_path, fastafile))])
    return subject_fastafiles

def query_list_fastafiles():
    """ Method Docstring """
    query_fastafiles = sorted_nicely([fastafile for fastafile in os.listdir(query_path) if os.path.isfile(os.path.join(query_path, fastafile))])
    return query_fastafiles

def filter_files_ending_with_one(sorted_files):
    """ Method Docstring """
    files_end_with_one = [name for name in subject_fastafiles if name[-1].isdigit() and not name[-2].isdigit() == 1]
    return files_end_with_one

You can simply return the function call, instead of assigning it to a variable and returning the variable, like so:

def subject_list_fastafiles():
    """
    Method Docstring
    """
    return sorted_nicely([fastafile for fastafile in os.listdir(SUBJECT_PATH) if os.path.isfile(os.path.join(SUBJECT_PATH, fastafile))])

def query_list_fastafiles():
    """
    Method Docstring
    """
    return sorted_nicely([fastafile for fastafile in os.listdir(QUERY_PATH) if os.path.isfile(os.path.join(QUERY_PATH, fastafile))])

def filter_files_ending_with_one():
    """
    The function filters the files end with 1
    """
    return [name for name in SUBJECT_FASTAFILES if name[-1].isdigit() and not name[-2].isdigit() == 1]

Main Guard

This is an excerpt from this fabulous StackOverflow answer.

When your script is run by passing it as a command to the Python interpreter,

python myscript.py

all of the code that is at indentation level 0 gets executed. Functions and classes that are defined are, well, defined, but none of their code gets run. Unlike other languages, there's no main() function that gets run automatically - the main() function is implicitly all the code at the top level.

In this case, the top-level code is an if block. __name__ is a built-in variable which evaluates to the name of the current module. However, if a module is being run directly (as in myscript.py above), then __name__ instead is set to the string "__main__". Thus, you can test whether your script is being run directly or being imported by something else by testing

if __name__ == "__main__":
    ...

If your script is being imported into another module, its various function and class definitions will be imported and its top-level code will be executed, but the code in the then-body of the if clause above won't get run as the condition is not met.

Updated Code

"""
Module Docstring (A description of your program goes here)
"""

import os
import re

SUBJECT_PATH = "/Users/catuf/Desktop/subject_fastafiles/"
QUERY_PATH = "/Users/catuf/Desktop/query_fastafiles"

def sorted_nicely(list_):
    """
    Sort the given iterable in the way that humans expect. https://blog.codinghorror.com/sorting-for-humans-natural-sort-order/
    """
    convert = lambda text: int(text) if text.isdigit() else text
    alphanum_key = lambda key: [convert(c) for c in re.split('([0-9]+)', key)]
    return sorted(list_, key=alphanum_key)

def subject_list_fastafiles():
    """
    Method Docstring
    """
    return sorted_nicely([fastafile for fastafile in os.listdir(SUBJECT_PATH) if os.path.isfile(os.path.join(SUBJECT_PATH, fastafile))])

def query_list_fastafiles():
    """
    Method Docstring
    """
    return sorted_nicely([fastafile for fastafile in os.listdir(QUERY_PATH) if os.path.isfile(os.path.join(QUERY_PATH, fastafile))])

def filter_files_ending_with_one():
    """
    The function filters the files end with 1
    """
    return [name for name in SUBJECT_FASTAFILES if name[-1].isdigit() and not name[-2].isdigit() == 1]

if __name__ == '__main__':

    SUBJECT_FASTAFILES = subject_list_fastafiles()
    QUERY_FASTAFILES = query_list_fastafiles()
    SUBJECT_FILES_ENDING_WITH_ONE = filter_files_ending_with_one()
\$\endgroup\$
1
  • \$\begingroup\$ Naming the parameter list_ is unnecessarily restrictive. The function actually works with any iterable. I would recommend it (for iterable), or simply x. Although I recognize that those are maybe also not perfect names, but naming is hard... \$\endgroup\$
    – Graipher
    Commented Sep 12, 2019 at 16:51
5
\$\begingroup\$

I'd suggest a couple minor stylistic tweaks:

  • Keyword arguments don't typically have spaces around the =, so we'd have sorted(l, key=alphanum_key) instead of sorted(l, key = alphanum_key)

  • You have a couple fairly long lines that could be broken up or reduce a little

  • Don't create a variable in one line if you're just going to return it in the next: just return the value directly: return sorted_nicely([...])

The major thing I'd suggest, since you explicitly call out Python 3.x, is to use pathlib (it was introduced in 3.4, but honestly nobody should be using any 3.x older than 3.4 anyway). It simplifies a lot of your os.path... logic very neatly using some well-implemented object-oriented programming. E.g., your current line:

[fastafile for fastafile in os.listdir(subject_path) if os.path.isfile(os.path.join(subject_path, fastafile))]

can be reduced to:

[fastafile for fastafile in subject_path.iterdir() if fastafile.is_file()]

You'd need to used pathlib.Path objects, like so:

from pathlib import Path

subject_path = Path('...')

You can read the complete documentation to get a better feel for Path objects, but for quick reference, you can access just the file's name as a string using my_path.name, which you can use for sorting.

The other major improvement is to use functions properly. Maybe I'm missing something, but query_list_fastafiles and subject_list_fastafiles appear to be exactly the same function, but operating on different inputs. This absolutely should be consolidated to a single function that takes the directory as an argument:

def sorted_nicely(l):
    """ Sort the given iterable in the way that humans expect. https://blog.codinghorror.com/sorting-for-humans-natural-sort-order/ """
    convert = lambda text: int(text) if text.isdigit() else text
    alphanum_key = lambda path: [ convert(c) for c in re.split('(\d+)', path.name) ]
    return sorted(l, key=alphanum_key)

def fastafiles(directory: Path) -> Iterable[Path]:
    return sorted_nicely([fastafile for fastafile in directory.iterdir() if fastafile.is_file()])

and then just call that function with different arguments:

subject_path = Path("/Users/catuf/Desktop/subject_fastafiles/")
query_path = Path("/Users/catuf/Desktop/query_fastafiles")

subject_fastafiles = fastafiles(subject_path)
query_fastafiles = fastafiles(query_path)
\$\endgroup\$
3
\$\begingroup\$

Style

All the other answers are quite good and you should definitely follow their recommendations. They do however gloss over a stylistic point I find particularly striking about your code: blank lines.

The way you use them makes them almost obsolete. There would not be a major difference in the readability of your code if you left them out, since there are almost distributed uniformly in your code, and are therefore not helping to structure it.

Python comes with an official Style Guide for Python Code (the infamous PEP8) and has clear guidelines on how to use vertical whitespace, aka blank lines to actually aid readability:

Quote from the section on Blank Lines (emphasis mine):

Surround top-level function and class definitions with two blank lines.

Method definitions inside a class are surrounded by a single blank line.

Extra blank lines may be used (sparingly) to separate groups of related functions. Blank lines may be omitted between a bunch of related one-liners (e.g. a set of dummy implementations).

Use blank lines in functions, sparingly, to indicate logical sections.

The code

Having SUBJECT_PATH and QUERY_PATH as module level constants is likely not a good idea. If they would ever change you will have to change your script. The first step to avoid that would be to change your function in order to accept input parameters instead of relying on a global variable. This part is covered in the answer of scnerd. The second logical step would be to have a look at Python's argparse module, which will help you to create an easy command-line interface for your script. argparse also has a great tutorial section where you can learn by example.

\$\endgroup\$
0
\$\begingroup\$

Call your sort method natural_sort -- or even better natsorted to comply with Python's sorted. The reason is that among programmers it is widely known under that name, so I recommend sticking with it. Jeff Atwood writes at the linked article:

It isn't called "Alphabetical sort"; it's collectively known as natural sort.

Furthermore, rather use existing software instead of reinventing the wheel if it's not for learning purposes. There's a natsort Python package available.

\$\endgroup\$
2
  • \$\begingroup\$ I have used the same function here. I have a question. It is a two line of code right? Do I need to use a package specifically for that?. How to decide when to use a package. \$\endgroup\$
    – catuf
    Commented Sep 13, 2019 at 16:50
  • \$\begingroup\$ @catuf Not sure, didn't verify your function thoroughly for all bugs. Generally, always use a package if it exists. Even the seemingly simplest function can be non-trivial to code, e.g. binary search! It's very easy to make index mistakes (off-by-ones). If you want to avoid the overhead later, then just integrate a minifier into your build system. \$\endgroup\$
    – ComFreek
    Commented Sep 14, 2019 at 16:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.