5
\$\begingroup\$

I have finished writing my nice and little text editor in Python with Tkinter. I tried to achieve the best user experience that I could.

  • I want this code to be 2/3 compatible, I tested it in both versions but if you could test it out again in the 2 versions too it would very knd of you.

  • I did not use a class on purpose because I think that there is no need for it in such a small project (< 100 lines), do you think I should use classes also in small projects like this?

  • Is the user going to use this without problems?
  • Is the code going to be readable if I look back at it after, let's say, a year?
  • Do I use good names?
  • I did not use a __name__ guard because this file is not meant to be imported, do you think I should use it anyway?
  • What other features could I implement?

The code:

try:
    import tkinter as tk
except:
    import Tkinter as tk

try:
    import tkMessageBox as pop_up
except:
    import tkinter.tkMessageBox as pop_up

import time

EMPTY_TITLE_ERROR_MESSAGE_SAVE = "Please write the name of the file you want to save in the given field."
EMPTY_TITLE_ERROR_MESSAGE_OPEN = "Please write the name of the file you want to open in the given field."
FILE_NOT_FOUND_ERROR_MESSAGE = "No file with the given title was found, remember that this text editor can only read files in its directory."
SAVING_SUCCESS_MESSAGE = "Your text is now stored in the {filename} file"
SIGNATURE_TXT_NOT_FOUND_MESSAGE = "Please be sure that the file you want to open exists and that it is in the same folder of this editor."


def _open():
    if not file_title.get():
        pop_up.showerror("Title is empty.",EMPTY_TITLE_ERROR_MESSAGE_OPEN)
        return 1

    if not ".txt" in file_title.get():
        filename = file_title.get() + ".txt"

    try:
        with open(filename) as f:
            main_text.delete("1.0",tk.END)
            main_text.insert(tk.INSERT, f.read(), "a")
    except IOError:
        pop_up.showerror("File not found.",FILE_NOT_FOUND_ERROR_MESSAGE)

def save():
    if not file_title.get():
        pop_up.showerror("No title.",EMPTY_TITLE_ERROR_MESSAGE_SAVE)
        return 1

    if not ".txt" in file_title.get():
        filename = file_title.get() + ".txt"

    with open(filename,"w+") as f:
        f.write(main_text.get(1.0,tk.END))
    pop_up.showinfo("File saved succesfully.",SAVING_SUCCESS_MESSAGE.format(filename=filename))

def add_date():
    full_date = time.localtime()
    day = str(full_date.tm_mday)
    month = str(full_date.tm_mon)
    year = str(full_date.tm_year)
    date = "\n"+day+'/'+month+'/'+year
    main_text.insert(tk.INSERT, date, "a")

def add_signature():
    try:
        with open("signature.txt") as f:
            main_text.insert(tk.INSERT, "\n"+f.read(), "a")
    except IOError:
        MESSAGE = SIGNATURE_TXT_NOT_FOUND_MESSAGE
        pop_up.showerror("\"signature.txt\" not found.",MESSAGE)  

root = tk.Tk()

menubar = tk.Menu(root)
menubar.add_command(label="Open", command=_open)
menubar.add_command(label="Save", command=save)
menubar.add_command(label="Add signature",command=add_signature)
menubar.add_command(label="Add date",command=add_date)


root.config(menu=menubar)

top = tk.Frame(root)
temp = tk.Label(root,text="Title:")
temp.pack(in_ = top,side=tk.LEFT)

file_title = tk.Entry(root)
file_title.pack(in_ = top,side=tk.RIGHT)

top.pack()

main_text = tk.Text(root)
main_text.pack()

tk.mainloop()
\$\endgroup\$
3
\$\begingroup\$

To answer your specific questions:

I did not use a class on purpose because I think that there is no need for it in such a small project (< 100 lines), do you think I should use classes also in small projects like this?

This is fine without classes, but on the other hand anything of any meaningful complexity will be much easier as OOP; why not have a go at rewriting this to use a class for the UI and see how it goes?

Is the user going to use this without problems?

I'd have thought so, but instructions and a test plan wouldn't go amiss!

Is the code going to be readable if I look back at it after, let's say, a year?

It would be nice to see some docstrings, but this is a pretty simple script and I expect you'd be able to follow it.

Do I use good names?

Mostly; see the other answers and below for exceptions, but it's generally clear what's going on. You could put all the boilerplate messages into a single dictionary to clean up the namespace a bit, too.

I did not use a __name__ guard because this file is not meant to be imported, do you think I should use it anyway?

Yes, it's good practice and you may later change your mind and import at least part of it.

What other features could I implement?

That is outside the scope of CodeReview, but what features do you like in a text editor?


Bare except is a very bad practice; if something unexpected goes wrong, you have no way of knowing what. Instead, be specific:

try:
    import tkinter as tk
except ImportError:
    import Tkinter as tk

You could alternatively consider:

import sys

if sys.version_info.major == 3:
    import tkinter as tk
    ...
else:
    import Tkinter as tk
    ...

This makes it more explicit what's going on.


The leading underscore on _open suggests that it should be considered private. I doubt this is your intention; if you just wanted to avoid the name conflict with the built-in function, the convention is a trailing underscore open_.


Tkinter comes with handy dialogs for loading and saving files: see tkFileDialog/tkinter.filedialog.

\$\endgroup\$
5
\$\begingroup\$

Style

Python has a guide style called PEP 8. If you have no good reason not to follow it, just follow it. You'll find various tools to check your code (pep8, pep8online.com/, etc) and even to fix it more or less automatically (autopep8).

Here's a list of problems detected :

txt_original.py:13:80: E501 line too long (105 > 79 characters)
txt_original.py:14:80: E501 line too long (105 > 79 characters)
txt_original.py:15:80: E501 line too long (141 > 79 characters)
txt_original.py:17:80: E501 line too long (138 > 79 characters)
txt_original.py:22:43: E231 missing whitespace after ','
txt_original.py:25:8: E713 test for membership should be 'not in'
txt_original.py:30:35: E231 missing whitespace after ','
txt_original.py:33:43: E231 missing whitespace after ','
txt_original.py:35:1: E302 expected 2 blank lines, found 1
txt_original.py:37:37: E231 missing whitespace after ','
txt_original.py:40:8: E713 test for membership should be 'not in'
txt_original.py:43:23: E231 missing whitespace after ','
txt_original.py:44:34: E231 missing whitespace after ','
txt_original.py:45:46: E231 missing whitespace after ','
txt_original.py:45:80: E501 line too long (95 > 79 characters)
txt_original.py:47:1: E302 expected 2 blank lines, found 1
txt_original.py:55:1: E302 expected 2 blank lines, found 1
txt_original.py:61:56: E231 missing whitespace after ','
txt_original.py:61:65: W291 trailing whitespace
txt_original.py:68:42: E231 missing whitespace after ','
txt_original.py:69:37: E231 missing whitespace after ','
txt_original.py:75:21: E231 missing whitespace after ','
txt_original.py:76:14: E251 unexpected spaces around keyword / parameter equals
txt_original.py:76:16: E251 unexpected spaces around keyword / parameter equals
txt_original.py:76:20: E231 missing whitespace after ','
txt_original.py:79:20: E251 unexpected spaces around keyword / parameter equals
txt_original.py:79:22: E251 unexpected spaces around keyword / parameter equals
txt_original.py:79:26: E231 missing whitespace after ','

And here's you code after most of the problems are fixed :

try:
    import tkinter as tk
except:
    import Tkinter as tk

try:
    import tkMessageBox as pop_up
except:
    import tkinter.tkMessageBox as pop_up

import time

EMPTY_TITLE_ERROR_MESSAGE_SAVE = "Please write the name of the file you want to save in the given field."
EMPTY_TITLE_ERROR_MESSAGE_OPEN = "Please write the name of the file you want to open in the given field."
FILE_NOT_FOUND_ERROR_MESSAGE = "No file with the given title was found, remember that this text editor can only read files in its directory."
SAVING_SUCCESS_MESSAGE = "Your text is now stored in the {filename} file"
SIGNATURE_TXT_NOT_FOUND_MESSAGE = "Please be sure that the file you want to open exists and that it is in the same folder of this editor."


def _open():
    if not file_title.get():
        pop_up.showerror("Title is empty.", EMPTY_TITLE_ERROR_MESSAGE_OPEN)
        return 1

    if ".txt" not in file_title.get():
        filename = file_title.get() + ".txt"

    try:
        with open(filename) as f:
            main_text.delete("1.0", tk.END)
            main_text.insert(tk.INSERT, f.read(), "a")
    except IOError:
        pop_up.showerror("File not found.", FILE_NOT_FOUND_ERROR_MESSAGE)


def save():
    if not file_title.get():
        pop_up.showerror("No title.", EMPTY_TITLE_ERROR_MESSAGE_SAVE)
        return 1

    if ".txt" not in file_title.get():
        filename = file_title.get() + ".txt"

    with open(filename, "w+") as f:
        f.write(main_text.get(1.0, tk.END))
    pop_up.showinfo(
        "File saved succesfully.",
        SAVING_SUCCESS_MESSAGE.format(
            filename=filename))


def add_date():
    full_date = time.localtime()
    day = str(full_date.tm_mday)
    month = str(full_date.tm_mon)
    year = str(full_date.tm_year)
    date = "\n" + day + '/' + month + '/' + year
    main_text.insert(tk.INSERT, date, "a")

def add_signature():
    try:
        with open("signature.txt") as f:
            main_text.insert(tk.INSERT, "\n" + f.read(), "a")
    except IOError:
        MESSAGE = SIGNATURE_TXT_NOT_FOUND_MESSAGE
        pop_up.showerror("\"signature.txt\" not found.", MESSAGE)

root = tk.Tk()

menubar = tk.Menu(root)
menubar.add_command(label="Open", command=_open)
menubar.add_command(label="Save", command=save)
menubar.add_command(label="Add signature", command=add_signature)
menubar.add_command(label="Add date", command=add_date)


root.config(menu=menubar)

top = tk.Frame(root)
temp = tk.Label(root, text="Title:")
temp.pack(in_=top, side=tk.LEFT)

file_title = tk.Entry(root)
file_title.pack(in_=top, side=tk.RIGHT)

top.pack()

main_text = tk.Text(root)
main_text.pack()

tk.mainloop()

As I am talking about tools, it is probably worth mentionning that you'll find various other tools to check your code.

pylint finds :

************* Module txt
C: 13, 0: Line too long (105/80) (line-too-long)
C: 14, 0: Line too long (105/80) (line-too-long)
C: 15, 0: Line too long (141/80) (line-too-long)
C: 17, 0: Line too long (138/80) (line-too-long)
C:  1, 0: Missing module docstring (missing-docstring)
W:  3, 0: No exception type(s) specified (bare-except)
F:  4, 4: Unable to import 'Tkinter' (import-error)
W:  8, 0: No exception type(s) specified (bare-except)
F:  7, 4: Unable to import 'tkMessageBox' (import-error)
E:  9, 4: No name 'tkMessageBox' in module 'tkinter' (no-name-in-module)
F:  9, 4: Unable to import 'tkinter.tkMessageBox' (import-error)
C: 20, 0: Missing function docstring (missing-docstring)
C: 29,31: Invalid variable name "f" (invalid-name)
C: 36, 0: Missing function docstring (missing-docstring)
C: 44,33: Invalid variable name "f" (invalid-name)
C: 52, 0: Missing function docstring (missing-docstring)
C: 61, 0: Missing function docstring (missing-docstring)
C: 63,38: Invalid variable name "f" (invalid-name)
C: 66, 8: Invalid variable name "MESSAGE" (invalid-name)
C: 69, 0: Invalid constant name "root" (invalid-name)
C: 71, 0: Invalid constant name "menubar" (invalid-name)
C: 80, 0: Invalid constant name "top" (invalid-name)
C: 81, 0: Invalid constant name "temp" (invalid-name)
C: 84, 0: Invalid constant name "file_title" (invalid-name)
C: 89, 0: Invalid constant name "main_text" (invalid-name)

but there is also pyflakes, pychecker, etc.

Logic around file name

The following logic :

if not ".txt" in file_title.get():
    filename = file_title.get() + ".txt"

seems wrong on many different levels :

  1. It does not follow PEP8 and should be if ".txt" not in file_title.get()
  2. You have the same hardcoded string in multiple places : you are pretty much asking to be biten at some point if you want to change this. What about TXT_EXTENSION = '.txt'.
  3. You are calling file_title.get() for no good reason
  4. You do not define filename if the condition in the if is evaluated to false. Maybe you meant :

    TXT_EXTENSION = '.txt' filename = file_title.get() if TXT_EXTENSION not in filename: filename = filename + TXT_EXTENSION

  5. You are changing "toto.tutu" to "toto.tutu.txt" but "toto.txt.tutu" does not change. If what you are interested in is extension, you should probably be using endswith.

    1. You have the same logic in multiple places : you should probably move it in a function of its own :

    def add_txt_extension_if_needed(filename): if not filename.endswith(TXT_EXTENSION): return filename + TXT_EXTENSION return filename

Date format

You can probably find in the documentation how to format a date as a string.

I have to go and I don't know tkinter but I guess other people will have additional relevant comments.

\$\endgroup\$
  • \$\begingroup\$ Autopep8 is very nice, thanks for suggesting it. \$\endgroup\$ – Caridorc Nov 18 '14 at 19:26
3
\$\begingroup\$

A few comments on top of @Josay's excellent review.


In this code:

if not file_title.get():
    pop_up.showerror("Title is empty.",EMPTY_TITLE_ERROR_MESSAGE_OPEN)
    return 1

if not ".txt" in file_title.get():
    filename = file_title.get() + ".txt"

There are 3 calls to file_title.get(). If you expect the returned value to be the same (I'm pretty sure yes), then it would be better to call it once and save in a variable. This is a good practice, not only as an optimization to reduce the number of calls, but also for extensibility: if you need to change something later, you will be do it in one place instead of 3.

And why do you return 1? What is the 1 for? If you mean to indicate "failure", then better to return False. But since you never return True, and the return value is not checked anywhere, you can just drop the "1" and return without argument.

You should also move all the code out of the global namespace and into a method, for example main, and call that method like this:

if __name__ == '__main__':
    main()
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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