Time Tracker Project

Question

I am working on a little Python project to track how much time I am spending coding and doing things outdoors. Coding subtracts and outdoors stuff adds.

My code is looking pretty bad. Any suggestions on how to make it better?

import tkinter as tk
import time
import json
import atexit
from flask import Flask

root = None
start_time = 0
end_time = 0
_add = False
_sub = False
entries_num = 0
_click = False
name = ''
_name = None
run_txt = None


def simplify(_time):
    return time.strftime('%H:%M:%S', time.localtime(_time))


def simple_mode(mode):
    if mode == 0:
        return "Add"
    elif mode == 1:
        return "Subtract"


def start():
    global start_time
    start_time = int(time.time())
    run_txt.configure(text="Running", fg="green")


def end():
    global end_time
    end_time = int(time.time())
    run_txt.configure(text="Not Running", fg="red")
    time_manager()


def find_mode():
    if _add and not _sub:
        return 0
    elif _sub and not _add:
        return 1


# 0 is add 1 is subtract
def time_manager():
    global entries_num
    entries_num += 1
    mode = simple_mode(find_mode())
    entry_raw = {'name': _name.get(), 'mode': mode, 'start': simplify(start_time), 'end': simplify(end_time), 'total_time': simplify(end_time-start_time-(-2211688800))}
    entries.update({str(entries_num): entry_raw})
    display()


def add():
    global _add
    global _sub
    _add = True
    _sub = False


def sub():
    global _add
    global _sub
    _add = False
    _sub = True


def display():
    height = 1
    width = 5
    cells = {}
    for i in range(height):  # Rows
        for j in range(width):  # Columns
            b = tk.Label(root, text="")
            b.grid(row=i + 16 + entries_num, column=j)
            cells[(i, j)] = b
    cells[(0, 0)]['text'] = entries[str(entries_num)]['name']
    cells[(0, 1)]['text'] = entries[str(entries_num)]['total_time']
    cells[(0, 2)]['text'] = entries[str(entries_num)]['mode']
    cells[(0, 3)]['text'] = entries[str(entries_num)]['start']
    cells[(0, 4)]['text'] = entries[str(entries_num)]['end']


class MainApplication(tk.Frame):
    def __init__(self, parent, *args, **kwargs):
        tk.Frame.__init__(self, parent, *args, **kwargs)
        self.parent = parent
        title_lbl = tk.Label(root)
        title_lbl['text'] = 'TimeTracker'
        title_lbl.configure(font=('Roboto', 20))
        title_lbl.grid(row=0, column=0, sticky='ew')
        srt_btn = tk.Button(root, text="Start", command=start, font=('Roboto', 8)).grid(row=10, column=0, sticky='sw')
        end_btn = tk.Button(root, text="End", command=end, font=('Roboto', 8)).grid(row=10, column=0, sticky='se')
        add_btn = tk.Button(root, text="Add Time", command=add, font=('Roboto', 8)).grid(row=11, column=0, sticky='se')
        sub_btn = tk.Button(root, text="Subtract Time", command=sub, font=('Roboto', 8))
        sub_btn.grid(row=12, column=0, sticky='se')
        global _name
        _name = tk.Entry(root, width=50)
        _name.grid(row=10, column=2)
        _name.insert(0, "Entry Name")
        _name.configure(state=tk.DISABLED)

        def on_click(event):
            _name.configure(state=tk.NORMAL)
            _name.delete(0, tk.END)

            # make the callback only work once
            _name.unbind('<Button-1>', on_click_id)

        on_click_id = _name.bind('<Button-1>', on_click)
        global run_txt
        run_txt = tk.Label(root)
        run_txt.configure(text="Not Running", fg="red")
        run_txt.grid(row=11, column=2)


def save():
    with open('data.json', 'w') as fb:
        _json = json.dumps(entries)
        fb.write(_json)


if __name__ == "__main__":
    root = tk.Tk()
    with open('data.json', 'r') as fr:
        text = fr.read()
        entries = json.loads(text)
        keys = entries.keys()
        for key in keys:
            entries_num = int(key)
            display()

    root.title("TimeTracker")
    root.minsize(200, 400)
    atexit.register(save)
    MainApplication(root).grid()
    root.mainloop()

Answer 1

The way you're handling add "modes" is a little clunky. It seems that only one of _add or _sub should ever be true at the same time. By making these two separate variables, you need to make sure that invariant is maintained everywhere. You've definitely helped make sure it's safe by using the add and sub functions, but those are band-aids on an awkward design.

The easiest way to simplify this is just to have one variable. Something like:

_is_adding = True

If this is true you're adding, if it's not, you're subtracting. Now find_mode can be much simpler:

def find_mode():
    return 0 if _is_adding else 1

And add and sub simply becomes:

def add():
    global is_adding
    is_adding = True

def sub():
    global is_adding
    is_adding = False

Although,

---

I think you're overusing globals here. In some cases, like UI callbacks, they may be the cleaner (but not only) option. find_mode for example though itself does not need to rely on globals. If you can cleanly pass data in instead of relying on globals, you should. I'd change it to:

def mode_number(is_adding: bool) -> int:  # I added some type hints in and renamed it
    return 0 if is_adding else 1

. . .

mode = simple_mode(mode_number(_is_adding))

The gain here is incredibly small because of how simple the functions involved are. In general though, it's a good habit to get into. Functions that explicitly take as arguments all the data they need, and return all the data that they produce are much easier to understand.

simple_mode and find_mode are kind of odd though if you think about them. Why translate add/sub into a number just to then translate it back to "Add"/"Subtract"? I think this makes more sense:

def format_mode(is_adding: bool) -> str:
    return "Add" if is_adding else "Subtracting"

. . .

mode = format_mode(_is_adding)

---

I think simplify is a bad name for a function. What's it "simplifying"? If I'm writing a function that turns something into a string, I usually call it format_* (although that's just a habit of mine). Give some description though. I'd probably call it format_time, and adding some simple type hints make the types clearer:

def format_time(_time: int) -> str:
    return time.strftime('%H:%M:%S', time.localtime(_time))

I'm not sure I 100% agree with calling the parameter _time to avoid the name collision, since a prefix _ typically means a "private" attribute, but I can't offhand think of a better name either, since the function is pretty broad in its purpose.

---

end_time-start_time-(-2211688800))

What the heck is -2211688800? That seems to offset the date by 70.1 years. Whatever it's purpose I see two problems:

• If that's meant to offset today's date to reach a certain format, it'll be outdated from the second it's written. You should see if you can automate the creation of that number.

• Magic numbers, especially large unusual like that should be named. Something like:

  TIME_OFFSET = 2211688800
  
  . . .
  
  end_time - start_time - (-TIME_OFFSET)
  

Also, x - (-y) is the same as x + y (unless x and y are some custom type with odd math laws). It should just be:

end_time - start_time + TIME_OFFSET

---

The Flask import seems to be unneeded.