7
\$\begingroup\$

I'm fairly new to object oriented programming and while I understand the basic concepts, I'm still having some trouble organizing my applications. I'm building a small Tkinter application as a learning experience and am trying to make sure I follow correct object oriented principles. I have a couple specific questions regarding what I've done so far (I appreciate any and all suggestions however!).

BACKGROUND INFO

The application will be to handle tasks that are written by the user and stored a database. The user should be able to use a search bar to look up tasks by key words or use radio buttons with a specified filter. The user can add any task they want and include relevant details that will be shown when the task is clicked on. The user will also be able to delete and update tasks as needed.

CONCERNS

  • Is it correct to use static methods often? I wanted to separate the logic from the rest of the application and I realized I'd be writing quite a few static methods from the way I structured my app
  • Would it be better to create a class for every widget separately? I created my Frame classes (see code below) and packed them with the widgets that goes into that specific frame. This causes the Frame classes to be quite large
  • I quite like the MVC architecture overall. Is it generally a good architecture to follow when designing GUI application?

It's my first time posting, so I apologize in advance if this is too much code or if I missing something important to add that would aid looking at my code. Please let me know if there's something I can add/remove that would clear things up.

WORKING CODE

import tkinter as tk
import sqlite3

# I kept the database in the global scope
con = sqlite3.connect('../database/a_db.db')
cur = con.cursor()

class Model:
    def __init__(self):
        pass

    @staticmethod
    def get_tasks():
        query = ''' SELECT * FROM tasks '''
        tasks = cur.execute(query).fetchall()
        return tasks

    @staticmethod
    def get_task_details(selected_task_id):
        query = '''SELECT * FROM tasks WHERE task_id=? '''
        task = cur.execute(query, (selected_task_id,))
        task_details = task.fetchall()[0][2]
        return task_details

    @staticmethod
    def get_task_status(selected_task_id):
        query = ''' SELECT assigned.emp_id FROM assigned
                    INNER JOIN tasks ON tasks.task_id = assigned.task_id
                    WHERE assigned.task_id=? '''

        db_active_task = cur.execute(query, (selected_task_id,)).fetchall()
        if len(db_active_task) > 0:
            return 'Assigned'
        else:
            return 'Not Assigned'

    @staticmethod
    def get_search(value):
        query_tasks = ''' SELECT task_id, task_name FROM tasks WHERE task_name LIKE ? OR task_description LIKE ? '''
        matched_tasks = cur.execute(query_tasks, ('%' + value + '%', '%' + value + '%'), ).fetchall()
        return matched_tasks


class View(tk.Tk):
    def __init__(self, controller):
        super().__init__()
        self.title("My App")
        self.middle_of_screen()
        self.resizable(False, False)
        self.iconbitmap(r'../images/an_icon.ico')

        self.controller = controller

        self.top_panel = TopFrame()
        self.left_panel = LeftFrame()
        self.right_panel = RightFrame(self.controller)


    def display_task_details(self, status, task_details):
        if self.left_panel.list_details.get(0):
            self.left_panel.list_details.delete(0, 'end')

        self.left_panel.list_details.insert(0, "Status: {}".format(status))
        if self.left_panel.list_details:
            self.left_panel.list_details.insert(1, "Task Description: {}".format(task_details))
        else:
            self.left_panel.list_details.insert(1, "Task Description: None")

    def display_tasks(self, tasks):
        self.left_panel.list_tasks.delete(0, 'end')
        for count, task in enumerate(tasks):
            self.left_panel.list_tasks.insert(count, '{}. {}'.format(task[0], task[1]))

    def middle_of_screen(self):
        window_width = 1350
        window_height = 750
        screen_width = self.winfo_screenwidth()
        screen_height = self.winfo_screenheight()

        x = (screen_width // 2) - (window_width // 2)
        y = (screen_height // 2) - (window_height // 2)

        self.geometry(f'{window_width}x{window_height}+{x}+{y}')

    def start(self):
        self.mainloop()


class TopFrame:
    def __init__(self):
        self.top_frame = tk.Frame(width=1350, height=70, relief='sunken', bg='#f8f8f8',
                                  borderwidth=2, padx=20)
        self.top_frame.pack(side='top', fill='x')

        # Task Button
        self.book_icon = tk.PhotoImage(file="../images/add_book.png")
        self.book_button = tk.Button(self.top_frame, text='Task\nManagement', image=self.book_icon, compound='left',
                                     font='arial 12 bold', bg='red', padx=10)
        self.book_button.pack(side='left', padx=(10, 0))

        # Employee Button
        self.users_icon = tk.PhotoImage(file='../images/users.png')
        self.employee_button = tk.Button(self.top_frame, text='Employee\nManagement', image=self.users_icon,
                                         compound='left', font='arial 12 bold', bg='red', padx=10)
        self.employee_button.pack(side='left', padx=(10, 0))

        # Add Employee To Project
        self.givetask_icon = tk.PhotoImage(file='../images/givebook.png')
        self.add_emp_button = tk.Button(self.top_frame, text='Assign Task \nTo Employee', image=self.givetask_icon,
                                        compound='left', font='arial 12 bold', bg='red', padx=10)
        self.add_emp_button.pack(side='left', padx=(10, 0))


class LeftFrame:
    def __init__(self):
        self.left_frame = tk.Frame(width=900, height=700, relief='sunken', bg='#e0f0f0',
                                   borderwidth=2)
        self.left_frame.pack(side='left')

        # TABS
        ## Tasks Tab
        self.tabs = ttk.Notebook(self.left_frame, width=900, height=660)
        self.tabs.pack()
        self.tab1_icon = tk.PhotoImage(file='../images/books.png')
        self.tab2_icon = tk.PhotoImage(file='../images/members.png')
        self.tab1 = ttk.Frame(self.tabs)
        self.tab2 = ttk.Frame(self.tabs)
        self.tabs.add(self.tab1, text='Task Management', image=self.tab1_icon, compound='left')
        self.tabs.add(self.tab2, text='Statistics', image=self.tab2_icon, compound='left')

        # List tasks
        self.list_tasks = tk.Listbox(self.tab1, width=40, height=30, bd=5, font='times 12 bold')
        self.scroll_bar = tk.Scrollbar(self.tab1, orient='vertical')
        self.list_tasks.grid(row=0, column=0, padx=(10, 0), pady=10, sticky='n')
        self.scroll_bar.config(command=self.list_tasks.yview)
        self.list_tasks.config(yscrollcommand=self.scroll_bar.set)
        self.scroll_bar.grid(row=0, column=0, sticky='nse')

        # List Details
        self.list_details = tk.Listbox(self.tab1, width=80, height=30, bd=5, font='times 12 bold')
        self.list_details.grid(row=0, column=1, padx=(10, 0), pady=10, sticky='n')

        # Statistics Tab
        self.tasks_count_label = tk.Label(self.tab2, text='Tasks Count', pady=20, font='verdana 14 bold underline')
        self.tasks_count_label.grid(row=0, sticky='e')
        self.employee_count_label = tk.Label(self.tab2, text='Emp. Count', pady=20, font='verdana 14 bold underline')
        self.employee_count_label.grid(row=1, sticky='e')
        self.assigned_tasks_count_label = tk.Label(self.tab2, text='Assigned Tasks\n Count', pady=20,
                                                   font='verdana 14 bold underline')
        self.assigned_tasks_count_label.grid(row=2, sticky='e')
        self.unassigned_tasks_count_label = tk.Label(self.tab2, text='Unassigned Tasks\n Count', pady=20,
                                                     font='verdana 14 bold underline')
        self.unassigned_tasks_count_label.grid(row=3, sticky='e')


class RightFrame:
    def __init__(self, controller):
        self.controller = controller
        self.right_frame = tk.Frame(width=450, height=700, relief='sunken', bg='#e0f0f0',
                                    borderwidth=2)
        self.right_frame.pack()

        self.search_bar = tk.Frame(self.right_frame, width=440, height=75, bg='#9bc9ff')
        self.search_bar.pack(fill='both')
        self.search_bar_entry = tk.Entry(self.search_bar, width=30, bd=10)
        self.search_bar_entry.grid(row=0, column=2, columnspan=3, padx=(30, 60), pady=10)

        self.search_bar_button = tk.Button(self.search_bar, text='Search', font='arial 12 bold', bg='#fcc324',
                                           fg='black', command=self.controller.show_search)
        self.search_bar_button.grid(row=0, column=0, padx=(60, 30), pady=10)

        # List Bar
        self.list_bar = tk.LabelFrame(self.right_frame, text='Filter', width=440, height=175, bg='#fcc324')
        self.list_bar.pack(fill='both')

        # Radio Buttons for List Bar
        self.list_choice = tk.IntVar()
        self.all_radio_btn = tk.Radiobutton(self.list_bar, text='All Tasks', var=self.list_choice, value=1,
                                            bg='#fcc324')
        self.completed_radio_btn = tk.Radiobutton(self.list_bar, text='Completed Tasks', var=self.list_choice, value=2,
                                                  bg='#fcc324')
        self.progressing_radio_btn = tk.Radiobutton(self.list_bar, text='Progressing Tasks', var=self.list_choice,
                                                    value=3,
                                                    bg='#fcc324')
        self.employees_radio_btn = tk.Radiobutton(self.list_bar, text='Current Employees', var=self.list_choice,
                                                  value=4,
                                                  bg='#fcc324')

        self.completed_radio_btn.grid(row=0, rowspan=2, column=1, sticky='nsw')
        self.progressing_radio_btn.grid(row=0, rowspan=2, column=2, sticky='nsw')
        self.all_radio_btn.grid(row=2, rowspan=2, column=1, sticky='nsw', pady=10)
        self.employees_radio_btn.grid(row=2, rowspan=2, column=2, sticky='nsw', pady=10)

        # Button for List Bar
        self.filter_list_btn = tk.Button(self.list_bar, text='Filter', fg='black', font='arial 12 bold', bg='red')
        self.filter_list_btn.grid(row=1, column=0, rowspan=2, sticky='nesw', padx=(10, 20), pady=25)

        # # Title and image
        self.image_bar = tk.Frame(self.right_frame, width=440, height=350)
        self.image_bar.pack(fill='both')
        self.title_right = tk.Label(self.image_bar, text='Welcome to the CTREL task \nmanagement system',
                                    font='arial 16 bold')
        self.title_right.grid(row=0)
        self.library_img = tk.PhotoImage(file='../images/library.png')  # Have to use self to keep reference!
        self.library_img_label = tk.Label(self.image_bar, image=self.library_img)
        self.library_img_label.grid(row=2)


class Controller:
    def __init__(self):
        self.model = Model()
        self.view = View(controller=self)
        self.show_tasks()
        self.view.left_panel.list_tasks.bind('<<ListboxSelect>>', self.show_task_details)

        self.view.start()

    def show_task_details(self, event):
        if self.view.left_panel.list_tasks.curselection():
            selected_task = self.view.left_panel.list_tasks.get(self.view.left_panel.list_tasks.curselection())
            selected_task_id = selected_task.split('.')[0]

            db_task_details = self.model.get_task_details(selected_task_id)
            status = self.model.get_task_status(selected_task_id)

            self.view.display_task_details(status, db_task_details)

    def show_tasks(self):
        tasks = self.model.get_tasks()
        self.view.display_tasks(tasks)

    def show_search(self):
        value = self.view.right_panel.search_bar_entry.get()
        searched_task = self.model.get_search(value)
        self.view.display_tasks(searched_task)


if __name__ == '__main__':
    c = Controller()
    c.view.start()
```
\$\endgroup\$
  • \$\begingroup\$ The use of the global scope is very much problematic. Your static methods aren't really static, they are instance methods that work on objects in global scope. Get rid of static methods, instantiate "global" objects within main, and pass them to the class's constructor. \$\endgroup\$ – Reinstate Monica Sep 19 '19 at 22:32
  • \$\begingroup\$ Hi! You successfully explained your concerns about the code but failed to tell us what the code does. What is your Tkinter app all about? Please edit your question to explain it. \$\endgroup\$ – 301_Moved_Permanently Sep 20 '19 at 7:29
  • \$\begingroup\$ @KubaOber Would it be more appropriate to instantiate the database in the Model class constructor as an alternative to instantiate through main? \$\endgroup\$ – ShockDoctor Sep 20 '19 at 18:08
  • \$\begingroup\$ @MathiasEttinger Hi! Thanks for the feedback, I've added a background section to explain at a high level what the app is accomplishing (and will accomplish). Please me know if anything is unclear! \$\endgroup\$ – ShockDoctor Sep 20 '19 at 18:09
  • \$\begingroup\$ @ShockDoctor I have no opinion, but for better composability, the looser the coupling, the better, so moving dependencies out helps with that. \$\endgroup\$ – Reinstate Monica Sep 20 '19 at 19:18
3
\$\begingroup\$

Problems with code run

  • imports

    I'd qualify the code as non-working. It forgets to import ttk

    from tkinter import ttk
    
  • DB structure.

    No table creation code found. We can only guess the db reading queries. Request to include table info next time.

  • Include sample repo

    For images etc, if we got the link to a sample repo, it might be easier to run the file.

Recommendations

Use SQLAlchemy

models.py

I highly recommend you use sqlalchemy. Since you are defining a custom model class, sqlalchemy's models are far more flexible.

from sqlalchemy import Column, String, Integer
from sqlalchemy.ext.declarative import declarative_base


Base = declarative_base()


class Task(Base):
    __tablename__ = 'tasks'

    id = Column(Integer, primary_key=True)
    task_name = Column(String)
    task_description = Column(String)

Then for queries it's a lot simpler:

session.query(Task).all()
session.query(Task).filter(record.id == some_id).first()
session.query(Task).like(search).all()

Why r''?

I guess you are using r in paths so as not to escape slashes. But forward slashes work on Windows as well.

A custom function for images

Since you are appending ../images each time, add a function to get the image path by just specifying the name:

import os


def image(item):
    dir_path = '../images'
    return os.path.join(dir_path, item)

Better class naming

class TopFrame: does not give info about the section. Maybe class OptionsFrame: might be better.

Better way to avoid long lines

Long lines like this:

        self.book_button = tk.Button(self.top_frame, text='Task\nManagement', image=self.book_icon, compound='left',
                                     font='arial 12 bold', bg='red', padx=10)

can be structured like this:

        self.book_button = tk.Button(
            self.top_frame, 
            text='Task\nManagement', 
            image=self.book_icon, 
            compound='left',
            font='arial 12 bold', 
            bg='red', 
            padx=10)

It makes the code more readable.

Group strings in one place.

Let's say you want to change the welcome message. You'd have to dig in the frame code.

Let's say you had a class:

class UIStrings:
    def __init__(self):
        welcome_message = '...'

Then you reference UIStrings.welcome_message in your UI code. This idea can be expanded for internationalisation, maybe using XML files.

Pass controller explicitly

Given

c = Controller()
c.view.start()

having:

controller = Controller()
view = View(controller=controller)

might suggest a cleaner architecture.

|improve this answer|||||
\$\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.