1
\$\begingroup\$

Greetings.

Lately I've tried to use a branch of text_formatting rules to enhance my productivity while working with different integration tools such as Git, but this formatting convention went so deep into my life that I thought that writing my To-Do list while adhering to it would be a good idea.

Since the element organizing section took me a lot of time everyday, I decided to write a simple organizer CLI application, while preventing object-oriented programming as much as possible ( Since I think that would add too much complexity for a simple application like this ) using Python argparse.

I would kindly accept feedback on:

  • Design patterns.

organo.py


#!/usr/bin/env python3
"""
KhodeNima's task organizer script.    
"""


from sys import argv as cli_arguments
from os import system as cmd_input
from pathlib import Path
from time import sleep
import webbrowser
import argparse
import sys


argument_parser = argparse.ArgumentParser(prog="Organo", usage="organize_tasks --elements [] --filename Path | organize_tasks [--help, -h, -?]", add_help=True)
command_line_arguments = cli_arguments[1:cli_arguments.index(cli_arguments[-1])+1]
command_line_argument_amount = len(command_line_arguments)


color_red = "\33[31m"
color_default = "\33[0m"


def handle_help_argument_action() -> None:
    if command_line_argument_amount == 2 and "-help" in command_line_arguments:
        argument_parser.print_help()
        sys.exit()

    if command_line_argument_amount == 2 and "-?" in command_line_arguments:
        argument_parser.print_help()
        sys.exit()


def setup_help_arguments() -> None:
    argument_parser.add_argument("-?", help="Same as --help.", action="help")


def setup_element_arguments() -> None:
    argument_parser.add_argument("--elements", help="The elements to organize.", required=True, nargs="+", action="append")


def handle_filename_argument_action() -> str:
    filename_signature = "--filename"
    filename = ''.join([argument for argument in command_line_arguments if command_line_arguments[command_line_arguments.index(filename_signature) + 1] == argument])

    return filename
    

def setup_filename_argument() -> None:
    argument_parser.add_argument("--filename", help="The filename or path you want to write to ( relative to current ).", action="store", required=True)


def main() -> None:
    cmd_input("clear")
    
    file_name = handle_filename_argument_action()

    try:
        with open(f"{Path.cwd()}/{file_name}", "w"):
            pass

    except PermissionError:
        print(f"{color_red} Invalid permission for writing to file, run the command as root.")
        sys.exit()

    header_title = str(input("Please enter your desired header title: "))
    todo_text = f"---- {header_title} ----\n\n\n"

    

    elements = []
    for element in command_line_arguments:
        if element.startswith("--") or element.startswith("-") or element == "--elements":
            continue

        
        if command_line_arguments.index(element) < command_line_arguments.index("--elements"):
            continue

        
        elements.append(element)


    for task_id, task_title in enumerate(elements):
        task_id += 1
            
        organized_task = f"    {task_id}. {task_title}. \n\n"
        todo_text += organized_task
        

        if task_title == cli_arguments[-1]:
            todo_text += "\n ---- END ----"


    saved_file_path = f"{Path.cwd()}/{file_name}"
    with open(saved_file_path, "w") as todo_file:
        todo_file.write(todo_text)


    cmd_input(f"touch {Path.cwd()}/{file_name}")
    cmd_input(f"chmod 700 {Path.cwd()}/{file_name}")


    color_syntax = "\33[32m"
    cmd_input("clear")
                        
    print(f"{color_syntax}DONE.\33[0m")
    sleep(2)


    cmd_input("clear")


    should_open_file = input("Do you want to open the file? ( Y OR N )?: ")
    if should_open_file.lower() == "yes" or should_open_file.lower() == "y":
        webbrowser.open(saved_file_path)


if __name__ == "__main__":
    setup_help_arguments()
    setup_element_arguments()
    setup_filename_argument()

    argument_parser.parse_args()
    handle_help_argument_action()
    main()

```
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Bonus points for that optional module docstring. (Though I suspect possibly pylint "made" you do it. I recommend preferring ruff, which doesn't get in your way, over pylint, which by default is much too picky and counter-productive.)

sort the imports

Your imports are weirdly organized, and not pep8 compliant. Don't give them a moment's thought. Just run isort and be done with it, move on to more interesting things.

globals

You define a bunch of global symbols.

argument_parser = ...
command_line_arguments = ...
command_line_argument_amount = ...
color_red = ...
color_default = ...

Maybe this is good? (Well, not that middle one, calling len() is too trivial to warrant defining it.) Maybe you really need these, and didn't want to bury them within a class? Ok, fair enough.

But at least mark them _private, e.g. _command_line_arguments. As written, you are inviting folks to import them into other modules, which does not appear to be your intent. We make an effort to keep the namespace somewhat clean, and scope things down within a smaller scope where feasible. That way there's less coupling, fewer things for the Gentle Reader to juggle in their head.

I will note in passing that pypi offers more than one library that will help with ANSI escape sequences, if you're willing to take an external dependency.

prefer Path

def handle_filename_argument_action() -> str:

Thank you for the type annotation.

Prefer Path over str here. It is more specific, so it more clearly communicates Author's Intent. And there are lots of nice convenience methods hanging off of each Path object.

filename = ...

Yeah, you wrote some code there.

It extends way, waayyy beyond an 80-char horizontal scroll limit.

I didn't read it.

If you want others to read what you wrote, write it legibly. You may find black helpful. As written, Author is essentially telling folks "don't read this!".

"Handle" is a rather vague verb. If this function is a getter, then prefer get_ in its name.

coupling

def setup_help_arguments() -> None:
    ...
def setup_element_arguments() -> None:
    ...
def setup_filename_argument() -> None:

These functions don't take any formal parameters. Yet we are passing argument_parser to them, implicitly, via the module namespace.

Don't do that. Prefer to explicitly name the arg parser in the signature, so the reader will quickly understand that we are evaluating the function for side effects on that object.

Consider consolidating these rather small functions into a single function.

Re-evaluate your decision to not define a class. When a bunch of functions accept similar argument, via signature or via the module namespace, that's the classic code smell that your functions are asking to be grouped together in a class, so they can share self.arg_parser.

DRY

Please DRY up handle_help_argument_action. For example you might choose to iterate in this way:

    for option in ['-?', '-help']:
        if ...

long function

I count 64 lines in main(). That's starting to get too long -- we need to vertically scroll in order to visually take it all in.

Consider breaking out one or more sections as (individually testable!) helper functions. Often for loops will be likely candidates.

Calling it main is not a terrible name. But it is a bit vague, and you neglected to offer a """docstring""" explaining what it does, its single responsibility.

unusual alias

from os import system as cmd_input

I recommend you just invoke the function as os.system(...). Giving it an unfamiliar name isn't helping others to understand what your code does. The os.system() documentation is telling you that, instead of this ancient call, you should prefer the newer subprocess.check_call(), which does a better job of conveniently accomplishing what you wish here.

And, oh look! You turned the usual sys.argv into cli_arguments. Less mysterious when someone reads it, but still, unnecessary.

Also, you defined ANSI escape codes for the color red, but not for clear screen?!? You'd rather the expense of forking off a /usr/bin/clear child? Ok, I mean it works. You might want to consider alternatives.

redundant type conversion

    ... = str(input( ... ))

We always get a str back from input(), so there's no need to stringify an existing string here.

Notice that if user hits CTRL/C here we leave behind a turd file, leftover from the write permission check. So consider deleting that file prior to soliciting input.

nested loops

    for element in command_line_arguments:
        ...
        if command_line_arguments.index(element) < command_line_arguments.index("--elements"):

That's a little weird. Typical idiom would be to set a bool flag when we encounter "--elements", and then just test the flag. (And for the number on the left that we're comparing, a for loop using enumerate would give you that index directly.)

As written, cost of scanning N arguments is O(N²) quadratic. We loop over N args, and for each arg we make linear scan of N args

Idioms like find . | xargs cmd... make "lots of args" a more common occurrence than you might at first think.

post-loop logic belongs after the loop

        if task_title == cli_arguments[-1]:
            todo_text += "\n ---- END ----"

That's just odd.

Elide the if, and exdent the assignment so it executes unconditionally.

If you're concerned about the "no elements" case, say so explicitly with an if elements: guard for the whole thing.

Path.resolve()

    saved_file_path = f"{Path.cwd()}/{file_name}"

Ok, I let it go the first time, but we keep seeing this idiom.

The type of file_name should be Path.

And then we should be assigning ... = file_name.resolve(). Or rather, up above we should have ensured that file_name is already fully resolved.

This is robust to things like a user entering a ../dir/file prefix, or entering a fully qualified pathname, common when pasting it in.

useless forks

    cmd_input(f"touch {Path.cwd()}/{file_name}")
    cmd_input(f"chmod 700 {Path.cwd()}/{file_name}")

I can't imagine what helpful effect that first one has, given that our previous action was to write to that same file. Elide it.

The chmod suffers from a TOCTTOU race bug. We could accomplish this more quickly with file_name.chmod(mode), but that would still suffer from the race condition. If you want to prohibit access for group+other, you need to do that in the open, which accepts an optional ..., mode= parameter.

And then doing a chmod afterwards would be a harmless belt-and-suspenders operation, just in case the file already existed with wrong permission.

The permissions check should similarly be careful with the mode parameter.

YORN

    if should_open_file.lower() == "yes" or should_open_file.lower() == "y":

You can DRY that up a little:

    if should_open_file.lower() in ["y", "yes"]:
\$\endgroup\$

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.