2
\$\begingroup\$

A friend of mine directed me to a variety of programming memes regarding the Greek question mark (;) and its similarity to a semicolon. This inspired me to create an incredibly cursed project (ironically in Python) that converts semicolons into Greek question marks in hopes of inducing as much programmer frustration as possible.

This is also my first time using the os module, so I'm not fully aware of best practices. Feel free to provide feedback!

import os, sys

def file_exists(path):
    return os.path.exists(path)

# Used to determine directory to place Greekified files
def specify_cwd():
    path = input("Specify a directory to create parsed files: ")
    if not file_exists(path):
        print("Not a directory!")
        path = specify_cwd()
    
    return path

# "Greekifies" files given a source file & cwd
def greekify(cwd: str):

    src = input("Specify a file to Greekify! ")

    if not file_exists(cwd + "/" + src): # Checks if file exists
        print ("File does not exist!")
        return

    file = os.path.basename(src)
    full_path = cwd + "/" + src
    
    file_basename, file_ext = os.path.splitext(file)

    if os.path.exists(cwd + "/greekified-" + file): # Ensures that no Greekified file already exists
        print("Greekified file already exists!")
        return
    else:
        try:
            with open(full_path, "r") as f:
                f_text = f.read().split("\n") # Splits file into list for each instance of \n

        except:
            print("Could not read file.")

    # Iterates through file text and replaces semicolons with Greek question marks
    greekified = map(lambda line: line.replace(";", "\u037e"), f_text) # Used unicode as to avoid confusion

    # Appends code to new file
    with open(cwd + "/greekified-" + file, "w") as nf:
        for line in greekified:
            nf.write(line + "\n")

        print("Successfully created Greekified file! Congrats!")

# Handles user text input
def fixed_inputs(text, pos_values: list=[]):
    val = input(text).lower()

    if val in pos_values or len(pos_values) == 0:
        return val
    else:
        fixed_inputs(text, pos_values)

def app():
    cwd = os.getcwd()

    print("Hello! This is the code Greekifier!")
    while True:
        print("Current directory: {0}".format(cwd))
        app_func = fixed_inputs("What would you like to do? (\"s\" to specify current directory / \"g\" to Greekify file / \"q\" to quit application) ", ["g", "s", "q"])

        if app_func == "s":
            cwd = specify_cwd()
        elif app_func == "g":
            greekify(cwd)
        elif app_func == "q":
            return

if __name__ == "__main__":
    app()
    sys.exit()
\$\endgroup\$
4
  • 1
    \$\begingroup\$ If you're curious, I wrote github.com/reinderien/mimic . Yes, it is cursed. \$\endgroup\$
    – Reinderien
    Mar 4 at 16:54
  • \$\begingroup\$ I would likely do this in PERL. \$\endgroup\$
    – Davislor
    Mar 5 at 3:22
  • 4
    \$\begingroup\$ ... or in sed: sed "s/;/;/g" helloworld.rs. \$\endgroup\$ Mar 5 at 10:29
  • 3
    \$\begingroup\$ The fact that this is 38 times larger than solutions in several other languages is perhaps a hint that Python isn't the optimal choice. (Of course, most of the code has nothing to do with the headline quest, and instead provides a basic file browser.) Consider sed (as @RichardNeumann) and perl, which both have an -i ("in place") option. Even without that option, you could use sed 's/;/;/g' <file >file.new$$ && mv file.new$$ file or sed 's/;/;/g' <file | slurp -o file. \$\endgroup\$ Mar 5 at 12:11

5 Answers 5

7
\$\begingroup\$

Remove Unnecessary Extra Function Call

You have:

def file_exists(path):
    return os.path.exists(path)

This is presumably so you can alias file_exists for the longer os.path.exists. Then why not just:

file_exists = os.path.exists
# or:
from os.path import exists as file_exists

This will result in one fewer function call.

Replace Tail Recursion With Iteration

You are using tail recursion in functions specify_cwd and fixed_inputs. Tail recursion can be replaced by iteration, which in my opinion is clearer and is often more efficient depending on what type of optimizations the language implementation applies to tail recursion. So, for example, specify_cwd could be expressed as:

def specify_cwd() -> str:
    while True:
        path = input("Specify a directory to create parsed files: ")
        if file_exists(path):
            return path
        print("Not a directory!")

Avoid Redundant Calculations

You have, for example, in function greekify:

if not file_exists(cwd + "/" + src):

Later on you have:

full_path = cwd + "/" + src

You are redundantly calculating the expression cwd + "/" + src since neither cwd nor src have changed values bewteen these two statements. You should move the assignment to full_path earlier and then reuse full_path later:

    full_path = cwd + os.sep + src
    if not file_exists(full_path): # Checks if file exists
       ...

    ...

        with open(full_path, "r") as f:

You Are Doing Unnecessary and Incorrect Line Splitting

Your goal is to replace all occurrences of the semicolon with the Greek question mark. What is gained by splitting the input text into lines and doing the replacement on a line-by-line basis? Moreover, after doing your replacement you re-write out the possibly updated line with:

nf.write(line + "\n")

But what if the last line of the input was not terminated with a newline character? Your "greekified" file now has a trailing newline character that was not in the input. Why not just:

    try:
        with open(full_path, "r") as f:
            f_text = f.read()
    except:
        print("Could not read file.")
        return # Missing return statement in original code

    greekified = f_text.replace(";", "\u037e")

    # Write to new file
    with open(greekified_file, "w") as nf:
        nf.write(greekified)

Note also that your original code was missing a return statement if an exception occurred reading the file.

Remove Unnecessary Call to sys.exit() and just have:

if __name__ == "__main__":
    app()
    #sys.exit()

Use Docstrings and Type Hinting Consistently Throughout Your Code

This will help document what each function is doing.

\$\endgroup\$
5
  • \$\begingroup\$ RE tail recursion: what you wrote is only true in language implementations that don’t implement it efficiently. Perhaps its worth being precise when you recommend against it here. \$\endgroup\$ Mar 4 at 23:09
  • \$\begingroup\$ Are you saying that tail recursion is implemented efficiently in Python? \$\endgroup\$
    – Booboo
    Mar 5 at 0:09
  • 1
    \$\begingroup\$ No; I suggest that you rephrase to say that tail recursion is not efficient in most typical Python implementations. The sentence as written is a bit too general and gives the impression that recursion should always be avoided in favor of loops. It’s the implicit “always” I dislike. \$\endgroup\$ Mar 5 at 0:26
  • \$\begingroup\$ @D.BenKnoble Okay, I have updated that section. \$\endgroup\$
    – Booboo
    Mar 5 at 3:05
  • \$\begingroup\$ A POSIX text file consists of a number of lines, each of which ends with a newline. If the file doesn't end with a newline (and isn't empty) then it's not a valid POSIX text file. Making such files is a horrible habit that I wish people would break. So I commend this method for fixing such files. \$\endgroup\$ Mar 5 at 11:56
5
\$\begingroup\$

There's a time-of-check/time-of-use (TOCTOU) condition here (as well as a useless comment that just repeats the code):

    if not file_exists(cwd + "/" + src): # Checks if file exists
        print ("File does not exist!")
        return

The file could be created or deleted between here and the open() call, so we need to handle failures at that point anyway, by catching the exception. When we do that, then this test becomes redundant.

The same goes for the other file we open, of course. We should be opening it in "x" mode instead of "w" so that it fails if the file already exists.

\$\endgroup\$
1
  • \$\begingroup\$ Nice, was just going to post an answer about the same issue. \$\endgroup\$
    – Matthew
    Mar 5 at 14:21
4
\$\begingroup\$

use Path

[I have added an implied type annotation.]

def file_exists(path: str):
    return os.path.exists(path)

It would be much nicer for path to be of Path. Then we could do the obvious if path.exists():

So e.g. specify_cwd() would return a Path.

Similarly we would have expressions like path.name, path.parent, and folder / src

Also cwd, current working directory, seems to be the Wrong name. Consider using os.getcwd() if that's what you really mean.

CLI

Rather than interactive prompting, consider going the Unix Tools route and accept a Path via sys.argv.

Of the several ways to do that, I feel import typer is simplest, and it would give you --help "for free".

while loop vs. recursion

Please don't have specify_cwd() invoke itself till obtaining a suitable input. Use a while loop to enforce the post-condition invariant.

A valid trace of execution could involve scanning an input stream of a few thousand invalid pathnames followed by a single good one. As written, the OP code would fail with RecursionError: maximum recursion depth exceeded. A simple while OTOH would do the Right Thing.

comments are for the Why, rather than the How

Let the code explain (in python) How we compute something. Elide useless # comments which shed no additional light on matters. Use comments to explain Why we chose a certain approach (in English).

docstring gives the Single responsibility

In several functions you followed this pattern:

# Handles user text input
def fixed_inputs( ... ):

where we should have seen:

def fixed_inputs( ... ):
    """Handles user text input."""

mutable default for parameter

def fixed_inputs( ... , pos_values: list=[]):

Please don't do that. Prefer to default it to None, and then assign pos_values = pos_values or None.

The pitfalls of mutable defaults are well known. No need to create a trap for some hapless maintenance engineer to fall into a few months down the road. After all, that person might be you!

format strings

Prefer an f-string here:

        print("Current directory: {0}".format(cwd))
        print(f"Current directory: {cwd}")

It is more legible, more maintainable.

no exit

Sartre tells us to elide the final line here:

if __name__ == "__main__":
    app()
    sys.exit()

BTW thank you, that's a nice __main__ guard.

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

Overview

The code layout is good, and you used meaningful names for functions and variables.

DRY

You can use a variable for the full path to the file instead of creating the path multiple times.

Lint check

pylint identified a few issues. There are some long lines that can be shortened.

You could split up the multiple imports on one line onto one line per import.

This line of code seems to be unused. Since the code seems to run fine without it, consider deleting it:

file_basename, file_ext = os.path.splitext(file)

Since trailing whitespace is not needed and can be annoying, just remove it.

You can simplify the following line using an f-string:

print("Current directory: {0}".format(cwd))

Naming

Short variable names like nf are not very descriptive. new_file is a little better.

Documentation

You could add a header comment at the top of the file which describes the purpose of the code, what it expects as input and what is creates as output.

It would be helpful to the user to print the full path to the output file.

This initial input prompt for the s option could use more explanation. I think it means that the user should specify the name of a directory where both the input file and output fill will reside. Since the line of code is already too long, the prompt text should be split out into multiple lines.

Consider using pyinputplus for user input as it might simplify some of your code.


Here is new code with some of the suggestions above:

'''
Replace semicolons with Greek question marks in a file.
Read in a file, then create a copy of the file with the replaced text.
'''

import os
import sys

def file_exists(path):
    return os.path.exists(path)

# Used to determine directory to place Greekified files
def specify_cwd():
    path = input("Specify a directory to create parsed files: ")
    if not file_exists(path):
        print("Not a directory!")
        path = specify_cwd()

    return path

# "Greekifies" files given a source file & cwd
def greekify(cwd: str):

    src = input("Specify a file to Greekify! ")

    if not file_exists(cwd + "/" + src): # Checks if file exists
        print ("File does not exist!")
        return

    file = os.path.basename(src)
    full_path = cwd + "/" + src

    file_path = cwd + "/greekified-" + file
    if os.path.exists(file_path): # Ensures that no Greekified file already exists
        print("Greekified file already exists!")
        return
    else:
        try:
            with open(full_path, "r") as f:
                f_text = f.read().split("\n") # Splits file into list for each instance of \n

        except:
            print("Could not read file.")

    # Iterates through file text and replaces semicolons with Greek question marks
    greekified = map(lambda line: line.replace(";", "\u037e"), f_text) # Used unicode as to avoid confusion

    # Appends code to new file
    with open(file_path, "w") as new_file:
        for line in greekified:
            new_file.write(line + "\n")

        print(f"Successfully created Greekified file {file_path}! Congrats!")

# Handles user text input
def fixed_inputs(text, pos_values: list=[]):
    val = input(text).lower()

    if val in pos_values or len(pos_values) == 0:
        return val
    else:
        fixed_inputs(text, pos_values)

def app():
    cwd = os.getcwd()

    print("Hello! This is the code Greekifier!")
    while True:
        print(f"Current directory: {cwd}")
        app_func = fixed_inputs("What would you like to do? (\"s\" to specify current directory / \"g\" to Greekify file / \"q\" to quit application) ", ["g", "s", "q"])

        if app_func == "s":
            cwd = specify_cwd()
        elif app_func == "g":
            greekify(cwd)
        elif app_func == "q":
            return

if __name__ == "__main__":
    app()
    sys.exit()
\$\endgroup\$
1
\$\begingroup\$

Consider this very brief replacement:

import fileinput

GREEK_Q = "\u037e"

for line in fileinput.input(inplace=True)
    print(line.replace(";", GREEK_Q), end="")

Besides being more succinct, mainly by using an existing library module, this behaves quite differently. Some changes are a matter of taste.

  • Use command-line arguments to specify which files to operate on. This makes interactive use easier, as you can use your shell's facilities like wildcard expansion, history search, tab completion, etc etc to avoid having to type in full file names. It also improves noninteractive use and reusing the code from other Python scripts.
  • With inplace=True it simply overwrites the input files. Perhaps add a backup='~' or similar to make it possible to revert this operation; or perhaps return to the original design of writing copies if the input files.

As an aside, your implementation needlessly reads each input file into memory in its entirety. This will unnecessarily make your program slow for large files. A common solution is to only read and write a line at a time. (fileinput does that for you behind the scenes.)

def greekify(...):
   ...
   with open(full_path, "r") as f, open(
           cwd + "/greekified-" + file, "w") as nf:
       for line in f:
           greekified = line.replace(";", "\u037e")
           nf.write(greekified)

This is an efficiency improvement, too, as we avoid looping over the same data more than once; you did it three times, one of which was an over-complicated lambda which should probably have been avoided by some other means if this didn't make it unnecessary anyway.

Also cwd is entirely unnecessary; the operating system already keeps track of which directory you are in, and resolves any file names which are not absolute relative to that. Perhaps see also What exactly is current working directory?

\$\endgroup\$

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