Implementing DataBase and Personalization #92

etiontdn · 2018-05-29T23:42:14Z

What issue are you solving? (Give #issuenumber)

How do you solve it? (please write this so a human can understand it - bulletpoints are okay)

Added database.py file with all the database actions
Added a window on the prog-o-meter.py PersonalizationGUI
Changed needed parts at user.py and prog-o-meter
Added Rainbow as a Color 😄 (Just for Fun)

If there is something specific in your solution, that you would like the reviewer to give feedback on, tell us here:

As before i still need to do some cleaning add docstrings and comments but 50% is completed
I never wrote docstrings or comments before so they may be not very good any suggestion is great
Take a look at the personalization window any suggestion to change is welcome
I took the freedom to make the window beautiful! 😃

If this is your first pull request to the prog-o-meter project, we would really appreciate it if you would fill out this Google form.
This lets us know more about our contributors, and what we can do to make it easier and nicer to contribute to the prog-o-meter project.
Please submit your answer here and put an 'x' in the box below.

It is my first pull request to prog-o-meter, and I have answered the survey
This is not my first contribution to the prog-o-meter

Fixed a bunch of bugs Added some docstrings and comments Needs to add more docstrings and comments and clean the code

Added the Default Button

Added more docstrings fixed them and added gitignore for PersonDB.db

etiontdn · 2018-06-01T00:59:49Z

I have a question about if i should add more things or leave it as is,
Any ideas?

lineaba · 2018-06-04T06:03:57Z

Hey @etiontdn I am so sorry it is taking me some time to give you proper feedback on this. I hope I will have time to do so tomorrow. Thank you for being patient.

lineaba

Hey, I have added a few comments to the first part of the code, did not have time to go through the entire thing tonight. I will continue tomorrow :)

lineaba · 2018-06-21T06:12:18Z

database.py

+again, as their names say they select and change the information in the database
+"""
+import sqlite3
+class DataBaser(object):


Could we change the name to Database? Is there a specific reason for the r? I think it could cause some confusion in the future, cause most people will probably try to spell it without the r?

Yes it could, but the name databaser is more beautiful 😄
The name is derived from words like driver "someone that drives or do all things needed to drive a car"
So databaser does all database things needed by the program.
But yes it can be changed for better understanding.

uuh, I like the idea, and I always enjoy playing with words, but I think for making it easy for people to use in the future, just calling it database might be the better way to go

lineaba · 2018-06-21T06:14:07Z

database.py

+
+    Attributes:
+        cdb: is the Connector to the DataBase,it creates a new file if it
+    doesnt exists as well


I think we can make this description a bit more clear. You write as well, but I am a little unsure about what it will be as well as? Is this function both for if there is no user already, and if a user exists, or only for new users?

It creates a New file if the .db file doesnt exist

okay, but the variable itself is only the connection right, and then you use that connection to create a new file - or did I misunderstand? Maybe the description of the variable can just be something like "the connection to the database" And then in the location where the file is actually created, we will comment it there. Would that make sense?

The sqlite3 database connector automatically creates a db file if there isn't one. but if comments can help to better understanding it can be added.

Maybe the wording can be something like:
cdb: the Connector to the DataBase. Will create a new database file if it doesn't exist already.

lineaba · 2018-06-21T06:15:35Z

database.py

+        creates a Table, if it exists uses try and except so error doesnt raise
+        """
+        self.cdb = sqlite3.connect('PersonDB.db')
+        try: 


It would be good with a comment on the try line, e.g. "try to establish a new person" or something more descriptive

It doesn't creates a new person it creates a table so it maybe could be

try: #try to create a new table And except: #if the table already exists don't raise error so the program isn't interrupted

yes, I like those suggestions, it makes it very clear what the code does :)

lineaba · 2018-06-21T06:17:14Z

database.py

+                     DAYSGOAL INT NOT NULL,
+                     MOTIVATION TEXT NOT NULL);''')
+            self.cdb.commit()
+        except sqlite3.OperationalError:


What happens to the program if an operational error occurs, and this step is passed? Will the program still be able to continue? If not, maybe we need to terminate or something here? I don't know a lot about working with databases. What kind of things could go wrong, to have us end up within the except state do you think?

The operational error occurs if you try to create a table with the same name of one that was already created.
When it passes it wont create a new one it just doesnt let the error interrupt the program.

lineaba · 2018-06-21T06:26:33Z

database.py

+        #Default Values:
+        primary_color = "#0000FF"
+        secondary_color = "#008000"
+        motivational_phrase = "Put Motivational phrases Here"


Is it possible for this to just be an empty string? Right now it displays this placeholder text on the canvas when the user haven't set their motivational phrase.

Yes, I just wanted to show to the user that he can put a motivational phrase there, but I think it came empty

Yes, we have to figure out how to best let the user know that they have that option, but the text makes it seem like the user can click it and add text (at least, they might think they can), so I think we need to think a bit more about how we can do it best. For now, let us just not display it, until they set a personal one :)

lineaba · 2018-06-21T06:38:25Z

database.py

+    def check_if_user_exists(self, person_name):
+        """Check if User exists in The DataBase
+        Args:
+            person_name: The Name of the person that will be searched in the DB


In the docstring of functions that return something, we also include:
returns:
[description of what it returns, e.g. "true if the user exists in the database, false if not"]

It only returns true because if the user name doesn't exists it creates a new one, I didn't set a return docstring but it could be added for better understanding

Okay, I think we should split this up into two different functions. Could we make a function that simply just checks if a user exists, and returns true if it does, and false if it does not. And then another function which creates a user? In the create-user-function we can then first check if the user exists (by calling the first function), and then only if false is returned (if user does not exist) will the function create a user. The second function doesn't need to return anything. We will need a function that can truly just check if a user exists and return true/false for some other parts of the program, so I think splitting them up would be really useful. What do you think?

lineaba · 2018-06-21T06:40:07Z

database.py

+        Args:
+            person_name: The Name of the person that will be searched in the DB
+        """
+        x = ""


Do you think we can find a more descriptive name for this variable, which will make the code easier for others to read? For example, username or name_holder

lineaba · 2018-06-21T06:40:43Z

database.py

+            person_name: The Name of the person that will be searched in the DB
+        """
+        x = ""
+        c = self.cdb.execute("SELECT * FROM PERSON\


I suggest that we call c cursor instead. I think it will make it easier for others to read. What do you think?

Yes it could be changed for better understanding.

lineaba · 2018-06-21T07:08:10Z

database.py

+        x = ""
+        c = self.cdb.execute("SELECT * FROM PERSON\
+                            where NAME='%s'" % person_name)
+        for row in c:        #For row in c(cursor)  x = row[0](NAME if select *)


I don't have much experience with database, so I am not sure if I understand this correctly, but what is the function of the loop? If you can go directly to row[0] as it looks like you are doing within the for-loop, would it maybe be possible to do it without a loop? Maybe I completely misunderstood something?

C selects part of the table where Name is equal person name.
Row is a row from the table where the first item is NAME so when row[0] is called it returns the name.
I'm not exactly sure if the cursor returns a dictionary or a list I will give it a try.

Okay, it seems to me that you can just go directly to calling the row, without looping. Maybe you'll have to do something like c.row[0] or something like that. Let me know what you find out, and I can try and look into it more myself too, if you want some help :)

From the little bit of looking at SQLite that I've had time to do tonight, I found this line in some code, and it seems like it is doing the same as what you want but without the for-loop. Maybe something like this could be done in our code too?
row = cursor.fetchone()

There are also fetchall and fetchmany, but since we are only interested in getting one line, I think fetchone should do the job.

lineaba · 2018-06-21T07:13:16Z

Hey @etiontdn I am sorry that it has taken me so long to get back to you, it has been some crazy week with finals and other stuff I had to fix. I really like how it looks when I run the program with your additions! I have started reviewing your code and giving some feedback, I didn't get all the way through, it is taking me some time, as I don't have much experience with databases. I will continue tomorrow, to look at the rest of the code. Let me know if you have any thoughts or questions about my comments:)

etiontdn · 2018-06-21T15:18:50Z

Thanks for the feedback, I will try to answer your comments but I can't make the changes right now.
I will make the changes as I can.

lineaba

hey, I have added some more comments, you can take a look at them when you have time. I had hoped to finish the review tonight, but it is getting late, and it is taking me some time to work through this, as I am new to both database and ttk. The last thing I haven't gone through yet is the user.py file, I will take a look and give my feedback on it as soon as possible :) Let me know if you have any questions or anything you would like to talk more about in the comments I have given you so far. :)

lineaba · 2018-06-22T05:36:25Z

database.py

+    def check_if_user_exists(self, person_name):
+        """Check if User exists in The DataBase
+        Args:
+            person_name: The Name of the person that will be searched in the DB


Okay, I think we should split this up into two different functions. Could we make a function that simply just checks if a user exists, and returns true if it does, and false if it does not. And then another function which creates a user? In the create-user-function we can then first check if the user exists (by calling the first function), and then only if false is returned (if user does not exist) will the function create a user. The second function doesn't need to return anything. We will need a function that can truly just check if a user exists and return true/false for some other parts of the program, so I think splitting them up would be really useful. What do you think?

lineaba · 2018-06-22T05:39:42Z

database.py

+        x = ""
+        c = self.cdb.execute("SELECT * FROM PERSON\
+                            where NAME='%s'" % person_name)
+        for row in c:        #For row in c(cursor)  x = row[0](NAME if select *)


Okay, it seems to me that you can just go directly to calling the row, without looping. Maybe you'll have to do something like c.row[0] or something like that. Let me know what you find out, and I can try and look into it more myself too, if you want some help :)

lineaba · 2018-06-22T05:43:05Z

database.py

+                            where NAME='%s'" % person_name)
+        for row in c:        #For row in c(cursor)  x = row[0](NAME if select *)
+            x = row[0]
+        if x == "":        #If x = ""(nothing) means no user with that name


great with the comment here. I think we can make it even more readable for humans, by saying something like "If no user with the name exists yet, create a new user with the name" or something like that :) I try to avoid writing any code like things in the comments (things like = and variable names), to make them more human-friendly. I know there are many different commenting styles, so no worries if you are used to doing it another way. I will make suggestions in places where I think we can avoid putting code in comments, but please let me know if you have questions or comments about the comment-styles of the project :)

Alright I think it is more human readable like that as well but I'm too biased towards math operators but I think these comments can be more readable

lineaba · 2018-06-22T05:43:43Z

database.py

+        if x == "":        #If x = ""(nothing) means no user with that name
+            self.new_person(person_name)        #Creates a new person
+
+        return True 


please see my comment for line 70 :)

lineaba · 2018-06-22T05:48:47Z

database.py

+        """
+        Get the number of days of the person with person_name
+        Args:
+            person_name: Name of the Person to get his number of days


Just two comments about docstring style. You are doing a really awesome job of adding one-line summaries to the docstrings, those are always the hardest ones for me to write, but yours are really good. Just remember to add a blank line after your summary line (the first line of the docstring) before any other lines (this is just part of the standard format, and means that if we are gonna pull all our docstrings out for documentation at some point in the future, a computer will be able to parse them automatically)
Also, for functions that return something, we need to include something like this:

returns:
number of days the user has completed

That way, other contributors get a quick overview about what the function does, which arguments it takes, and what it returns

Instead of saying "his number of days" it would be great if we could say "their number of days", so that the code is gender neutral :)

Yes, I will try to add a short return description in the docstrings.
And I will change words for better gender neutrality.

lineaba · 2018-06-22T07:13:54Z

prog-o-meter.py

+        defaults_button.grid(row=1, column=0, sticky="WE")
+        apply_button = ttk.Button(self.root,
+                                  text="Apply",command=self.apply_values)
+        apply_button.grid(row=1, column=1, sticky="WE")


I haven't worked with ttk, so I am excited to learn more about it. Would you mind explaining what the sticky keyword does? :)

It makes the widget take all the space he can from the sides:
wens that are respectively west, east, north and south.

cool, thanks :)

lineaba · 2018-06-22T07:20:23Z

prog-o-meter.py

+        self.secondary = Tk.IntVar()
+        self.primary.set(colorslist[self.user.get_color_primary()])
+        self.secondary.set(colorslist[self.user.get_color_secondary()])
+        c=0        #amount of columns


In these lines (479-481) I would suggest changing the variable names. It will take up a bit more space to write them every time, but it makes it easier to read, and then we don't need to add that comments that explain what the variable is, because the name will do it automatically. What do you think?

Yes I will try to make better variable names.

lineaba · 2018-06-22T07:21:02Z

prog-o-meter.py

+        c=0        #amount of columns
+        r=1        #row number
+        v=0        #the number value
+        for color in colors:    #a for loop that creates the buttons


awesome that added a comment here! :)

lineaba · 2018-06-22T07:23:16Z

prog-o-meter.py

+                        variable=self.primary, value=v).grid(row=r, column=c)
+            v = v + 1
+            c = c + 1
+        c = 0


Just to make sure I understand the code (sorry, it is getting late at night here, my brain is not so sharp anymore), would you mind explaining why the values are reset here, and why some are set to 0 and some are set to 3? I am sure it is probably obvious, but I appreciate the help keeping track of everything:)

To make different columns and rows for the grid, I think I need to change var names there as well.

lineaba · 2018-06-22T07:25:08Z

prog-o-meter.py

+        v = 0
+        self.secondary_label = ttk.Label(self.colors_tab, text="Secondary Color")
+        self.secondary_label.grid(row=2, column=4)
+        for color in colors:        #another for loop instead of copy&paste


this comment is good at explaining your choice, but since the code will constantly keep changing when people are adding more and more stuff, as time passes, it may become a little unclear what this references to. I think it might be better to just explain what the for-loop does, is that ok with you? :)

etiontdn · 2018-06-22T15:09:58Z

I think I miss clicked something.

lineaba · 2018-06-22T20:05:30Z

@etiontdn I am not sure I understand your last comment, what makes you think you misclicked something?

etiontdn · 2018-06-22T21:49:38Z

I think it is fixed now. I don't have a computer available right now, so I'm using my phone and I miss clicked a button.

lineaba · 2018-06-24T23:37:31Z

user.py

@@ -16,7 +16,7 @@
 any other part of the program, but is included in order to accomodate
 their inclusion in the future (see https://github.com/lineaba/prog-o-meter/issues/33).
 """
-
+from database import DataBaser as DBser


If you change the name of the database object, remember to also change it here :)

lineaba · 2018-06-25T00:08:41Z

I started looking through the user.py changes in your pull request, and I am wondering if there is still a use for this module, since a lot of the functions are created in your database file, and it seems like the user.py functions are more wrappers around the database functions. So I am wondering if we can possibly use the database functions directly, rather than the user functions, I think that would be easier to track for a beginner.
@garroadran would you mind taking a look at the proposed changes and let me know what you think? Now that we have a database in the building, do we still have a need for the user module?

dliberat · 2018-06-25T04:19:51Z

@lineaba
I'm afraid I don't have time to look through this PR in detail, but at first glance I would say that everything is getting more complicated than it needs to, here.

If the database/databaser object is negotiating input/output from the database, then it seems to me like there's a couple of different ways it could be organized.

Create a connection to the database and retrieve a user.
In a highly simplified form, it might look like this:

# User types in or selects a name
name = input('Type in your name')

# Create the connection to the DB, then retrieve a User object
db = Databaser() 
newuser = db.get(name)

# Do whatever work needs to be done with the user object, then save to db as necessary
newuser.add_days()
db.save(newuser)

Create a user, and pass in a database controller so that the user can handle everything itself

# Create the connection to the db
db = Databaser()

# Pass in an instance of the database when creating the user. If the username exists in the DB,
# then the DB retrieves the data. Otherwise, it creates a new user in the DB.
newuser = User('David', db)

If you do something like that, then inside the User class you can have something like

def __init__(username, db_controller):
    self.db = db_controller
    self.display_name = username

    if db.exists(username):
        self.load_attributes_from_db()
    else:
        self.db.create_new(self)
        self.load_attributes_from_db()

# Methods that modify the user's attributes automatically save the updated data to the db
def add_days(count=1):
       self.days += count
       self.db.save(self)

Obviously those aren't the only two options, but they are the first two ways that come to mind. Personally, I think something like #2 is an easy way to work.

Sorry, but I'm not going to be able to really devote much time to this issue. I still think that you are going to want to think about having a User of the system, and a Database object for controlling the saving/retrieving of data.

What I would recommend is thinking very specifically about how you want to interact with the different parts of the program, and decide how to organize things depending on what you think is a convenient and easy-to-understand way of working with the different entities in the program.

lineaba · 2018-06-27T04:07:49Z

Thank you @garroadran, I appreciate your input, and both ideas look interesting. I understand that you don't have time to help, but if you got a moment, I just have one question about your second code example.
Would you suggest that when the save function is called, then the entire user-model (sorry if I am not getting the terminology right here) is updated, i.e. all "fields" for the user, goal, days_left, name, etc. is set to the value that is the current value in the user-object from user.py?
Or would it make sense to just read all the info from the database (or create a user in the database if they don't already exist) when the program is started, and then do all the work as we used to, just storing values in user.py, and then wait to update the database until the program is closed, instead of everytime an update to the user-object (in user.py) is made.
Again, I understand if you won't have time to respond to this, but I just thought I would ask.

@etiontdn I will be looking more into SQLite (and maybe try to see if I can find some code examples to learn from) so I can better help find the best solutions for us. I am sorry it will take a while before I can give some useful feedback :)

etiontdn · 2018-07-05T22:35:18Z

I have not been active for some time, i don't understood what changes to do from the latest discussion but i will start making the other changes.

lineaba · 2018-07-05T23:44:39Z

Hey etion, I have not been very active either, my apologies. What time zone are you on? I was thinking it might be easier for us to Skype to talk about how to handle the database than to message back an forward. I have looked into databases, and think I have a suggestion about how we could do it, but I would like to hear your thoughts. If skyping doesn't work, we can just keep messaging, but let me know. I am on the Westcoast of America (GMT-7)

…

On Thu, Jul 5, 2018, 15:35 etiontdn ***@***.***> wrote: I have not been active for some time, i don't understood what changes to do from the latest discussion but i will start making the other changes. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#92 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AfZFL_cPlOW3g57cpzMaUF-dHteI85eXks5uDpSmgaJpZM4USYWG> .

etiontdn and others added 18 commits May 20, 2018 21:28

Added DB

d651742

Deleted a file taht i added

b870cb4

Added Some Docstrings and comments for clearer code

64c8967

Fixed Local Emacs file Error

8a0190a

Added more comments

8e8dd20

removed unnecessary file

c5564f8

savetocloud

63c4b84

Almost there

8a6100b

Fixed a bunch of bugs Added some docstrings and comments Needs to add more docstrings and comments and clean the code

Merge branch 'master' of https://github.com/etiontdn/prog-o-meter

7a36aa5

deleted files

4adc79a

Added a Button i forgot

6672fbe

Added the Default Button

Delete file error

0ef40a9

Corrected colors

74052a2

a

7c5ac55

some colorfixes

f336067

Delete db

23ef775

Merge branch 'master' of https://github.com/etiontdn/prog-o-meter

65e73fa

Clear docstrings and comments

0bb4400

Added more docstrings fixed them and added gitignore for PersonDB.db

lineaba reviewed Jun 21, 2018

View reviewed changes

lineaba reviewed Jun 22, 2018

View reviewed changes

lineaba reviewed Jun 25, 2018

View reviewed changes

Implementing DataBase and Personalization #92

Are you sure you want to change the base?

Implementing DataBase and Personalization #92

Conversation

etiontdn commented May 29, 2018 • edited

etiontdn commented Jun 1, 2018

lineaba commented Jun 4, 2018

lineaba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lineaba Jun 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lineaba Jun 27, 2018 • edited

Choose a reason for hiding this comment

lineaba commented Jun 21, 2018

etiontdn commented Jun 21, 2018

lineaba left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etiontdn commented Jun 22, 2018 • edited

lineaba commented Jun 22, 2018

etiontdn commented Jun 22, 2018

Choose a reason for hiding this comment

lineaba commented Jun 25, 2018

dliberat commented Jun 25, 2018 • edited

lineaba commented Jun 27, 2018

etiontdn commented Jul 5, 2018

lineaba commented Jul 5, 2018 via email

etiontdn commented May 29, 2018 •

edited

lineaba Jun 24, 2018 •

edited

lineaba Jun 27, 2018 •

edited

lineaba left a comment •

edited

etiontdn commented Jun 22, 2018 •

edited

dliberat commented Jun 25, 2018 •

edited