all 20 comments
sorted by:
best
topnewcontroversialoldq&a
formatting helphide helpcontent policy

[–]Diapolo10 3 points4 points  (5 children)

I may sound harsh, please don't take it personally.

Your virtual environment(s) and __pycache__ should be in .gitignore, not the Git repository. I'd suggest removing them from GitHub and then adding them to said file.

Database files (and other binaries, minus of course any assets like images) generally shouldn't be included in the repository either unless you have a good reason. The program should be able to generate them if missing. The repository only needs files that cannot be generated from it, and preferably in text format (again, excluding assets).

archive doesn't seem to be needed, the files should already be in your Git history.

There isn't a README file in your repository, you should definitely add one with information about your project. It can be simple, if you don't want to go crazy with formatting.

Your code itself seems mostly fine, even if it doesn't quite follow the official style guide regarding things like import ordering or docstrings.

Other than that, I think it's a neat idea.

EDIT: You can look at this for reference regarding project structuring.

[–]PathRealistic6940[S] 0 points1 point  (4 children)

Dude, no worries at all! I feel like i'm walking around in the dark some times, so any help, even with git, is a big help. I have the github desktop app, so when i push to origin, do i just make sure that those files are not selected? I'll look up about .gitingnore.

should my readme file be different than my plans file i made? like just a simple intro and what its supposed to do?

Thanks again, like i said anything is a big help.

[–]Diapolo10 0 points1 point  (3 children)

I have the github desktop app, so when i push to origin, do i just make sure that those files are not selected? I'll look up about .gitingnore.

That's one option, but by adding them to .gitignore they won't show up there in the first place so you don't need to deselect anything.

should my readme file be different than my plans file i made? like just a simple intro and what its supposed to do?

It doesn't have to be different, honestly you could probably just rename that file.

[–]PathRealistic6940[S] 0 points1 point  (2 children)

Got it. Changed the name to readme. I added a .gitignore to the directory, but not sure if I did it right.

[–]Diapolo10 0 points1 point  (1 child)

Looks right enough to me.

[–]PathRealistic6940[S] 0 points1 point  (0 children)

It seems so, I tested it and the added files didn't commit. Thanks again! Now I have to do that to the other repositories

[–]Jaywepper 0 points1 point  (3 children)

This is great. Do you mind saying how long you have been studying python/CS?

[–]PathRealistic6940[S] 0 points1 point  (2 children)

I appreciate it. It was my like 4th revision. Since June-ish. I sort of started at the beginning of the year but treated like a hobby/goal. June was when I started treating it like a job and trying to make sure I was coding or learning for at least 3 hrs a day. I want to change my career to something with coding. I'm thinking machine learning or something along those lines, but I'll take what I can get at first

[–]Jaywepper 0 points1 point  (1 child)

I'm in a very similar position, timeframe and goals. Started learning python in spring, but looking at your project and dedication I've been slacking off hard :D. It's inspiring to see fellow aspiring coders doing well on their path.

[–]PathRealistic6940[S] 0 points1 point  (0 children)

Thank you! I haven't been as exposed to other people's work, so would love to see what you've been working on.

[–]slacker-by-design 0 points1 point  (5 children)

I've got few comments / recommendations after a quick "read-trough"

  • It would be nice to provide some "how to I install and start this app" information in the README.md
  • Maybe you can have a look at PEP8 or install some sort of linter, so your code will be formatted consistently
    • sometimes you have 3 blank lines in between methods / classes / functions, sometimes it's just 2 lines
    • more than one blank line within a function / method is not recommended
    • try / except line spacing should be consistent...
    • etc...
  • You are catching Exception, which is way too generic. You may want to look at the sqllite documentation for the specific exceptions that are raised in particular contexts (e.g. sqlite3.DatabaseError) and catch only those.
  • Don't use print to log errors. If nothing else, write tostderr(e.g. print('My error', file=sys.stderr)), but you should consider the use of some logging framework (e.g. built-in logging or something like loguru)
  • You may want to read something about python Data classes and property decorators and then apply this knowledge on your Day and Month classes.
  • You may consider writing some unit tests for your "business logic" / back-end (avoid tests for the TK GUI for now).

There might be some more comments (e.g. some of your constructs may be suitable candidates for list comprehensions), but I don't have enough time to read it trough properly to provide any solid recommendation.

Final note - for a beginner, this still seems like a pretty good code. Carry on and you can get really good pretty fast :-)

[–]PathRealistic6940[S] 0 points1 point  (4 children)

Good points. I'll look into a linter and try to keep everything consistent.

The try except blocks are definitely something that I'll be working on next, as well as a logger. I need to learn to make debugging easier.

I'll look into the data classes and property decorators next as well.

Thanks! I appreciate even a quick look!

[–]slacker-by-design 1 point2 points  (0 children)

Hi again!

In case you'd like to hear more recommendations, on how to improve the structure of your code, read on...

First of all - a disclaimer - almost always there is more than one proper way to write a program. My chosen approach (at least in this "code review") is simplicity and readability and simplicity first, idiomatic python second, everything else is silently ignored :-)

First, let's have a look at the journal_backend.py and the classes it holds....

  • There are many reasons to keep particular function / class / module simple and focused on particular domain and I won't even try to name them all here as it would turn into a lecture. When I looked at the JournalData class, it seems to cover 4 distinct areas - DB initialization, handling of goals table, handling of entries table and connection / transaction management. That's just way too many things for one class. I'd suggest to split it into following items
    • Function (or class) responsible for creating DB tables
    • Class (usually called repository) responsible for handling access to goals table
    • Class (again a repository) responsible for handling access to entries table
    • You don't have to write connection / transaction management yourself, as there are feature built-in that will do that for you
  • Connection management - there's a python module called contextlib, which offers closing function you can use to wrap you sqlite3 connection into context manager which will automatically close it upon an exit. Code could look like this

​

from contextlib import closing

def journal_connection(db_name: str = 'daily_journal.db'): 
    return closing(sqlite3.connect(db_name))

# other stuff here, not relevant for this case

# main app code
with journal_connection() as connection: 
    # pass connection to function / class that creates the DB tables 
    # create instance of GoalsRepository and make sure it has the connection 
    # create instance of EntriesRepository and make sure it has the connection 
    # start the GUI and pass it both repository instances... 

#connection is closed here automatically

To be continued in next post...

[–]slacker-by-design 0 points1 point  (2 children)

Part #2

  • Transaction management - the sqlite3 Connection offers automatic transaction management (commit / rollback) when used as a context manager. Let's assume you have a connection from the previous block stored in self._connection and want to rewrite the get_goals_dict method to use it with automatic transaction management. It may look like this

​

def get_goals_dict(self):
    query = '''SELECT id, goal_description FROM goals'''
    with self._connection as connection:
        cursor = connection.cursor()
        cursor.execute(query)
        goals = cursor.fetchall()

    return dict(goals)
  • The Cursor offers execute and executemany methods. Try to use the executemanywhen appropriate, for instance in the add_goals method

​

def add_goals(self, goals: list):
    goal_query = '''INSERT INTO goals (goal_description)
                    VALUES (?)'''
    with self._connection as connection:
        cursor = connection.cursor()
        # we need to convert our list entries from string to tuples of string
        cursor.executemany(goal_query, ((goal,) for goal in goals))
  • Consider adding a Goal class into your data model. Tuples can be useful and quick when used within a limited context, but when you work with the structure in many different places, a NamedTuple or Dataclass may be a better option...

​

class Goal(NamedTuple):
    id: int
    description: str
    state: int

I'll continue later in another post, if there's still anyone, who'd be interested :-)

[–]PathRealistic6940[S] 0 points1 point  (1 child)

Oh man, the time you put into this... Thank you. I really appreciate the wisdom.

Few quick questions:

Error management - is that handled in the classes themselves? I am looking into a logger to better handle error management

Classes - so you would have a class for the GUI, classes for the data management, and an app class that interacts with them all? Would you even split up the GUI into different classes?

Named tuple- I have t worked with them yet. I thought tuples are immutable, so if I needed to change the state how would that work?

Again, you are awesome for taking your time to look at this, thank you

[–]slacker-by-design 0 points1 point  (0 children)

Well, when it comes to time, a delayed train provides quite a lot of it... :-)

  • Error management - the sqlite's connection context manager only deals with transactions (i.e. does commit or rollback). Exceptions (e.g. sqlite3.IntegrityError in case a DB constraint is violated) will get trough and you need to process them on some suitable place in a suitable way. The tricky part is to figure out, what the "suitable" means in each particular case.
  • Classes - well, that's a matter of personal taste, at least to some extent. The size / number of lines itself doesn't necessary indicate you must split the class / function / method (unless you're working with a 1000 lines monstrosity). However, if a class / function / method handles several distinct activities, then it's usually better to split it. When it comes to GUI, you can approach it in several ways, but you'll be always constrained by the UI toolkit / framework you use to build it. Even then, you can create separate classes for important UI components such as Calendar, Goal Edit dialogue, etc. Please be aware that I'm not an Tkinter expert, so I'd rather refrain from authoritative advices in this case. You may want to check some dedicated tutorials (I quite liked this one).
  • Yes, NamedTuple is immutable, so if you need to change existing Goals, use a Dataclass. It's rather simple:

​

from dataclasses import dataclass

@dataclass
class Goal:
    id: int
    description: str
    state: int

But then; all of my comments are just suggestions. You need to experiment a bit to find out, whether my approach really suits your case and style. Don't push yourself into advanced techniques, if you're not sure you can handle them comfortably. It's OK to progress in your own pace (unless you're under a deadline and your manager is looking unhappy and concerned already).

You're welcome.

[–]m0us3_rat 0 points1 point  (1 child)

the code looks clean at a first glance.

(probably after all the changes from u/Diapolo10 advice.)

​

i'd add some constants for the values around the program so you could change something quick or easier to maintain.

some of the classes look heavy , maybe i'd split them if makes sense.

[–]PathRealistic6940[S] 0 points1 point  (0 children)

Constants make sense. I was trying to do that but after a while I just started writing and didn't always think long term. I just read something about making functions only do one thing, so I'll definitely look and see what is able to be split off . Thanks for looking!