Opinions always differ on matters like this, but your
UserService class seems
like a severe case of over-engineering.
First, three of the methods seem to offer nothing very substantive:
reactive_user() just set boolean attributes on a
User instance; and
is_active() just returns an attribute. I would encourage
you to simplify things: just operate on the user instance directly.
update_activity_tracking() does more stuff to the user instance,
but there’s no obvious reason not to move the method into the
User class as well.
update_password() is a good example of a situation where you could
use a property, because you need to perform some calculations before storing
the attribute. Here’s the basic pattern:
def __init__(self, password):
# Initialize an under-the-hood attribute.
self._password = None
# Call the setter.
self.password = password
# The getter just returns it.
def password(self, new_password):
# The setter does the needed calculation.
self._password = get_hashed_password(new_password)
register_user() method is a natural fit for a
def register_user(cls, email, password):
# Python allows you to call functions in keyword style even if the
# function defines arguments as required positionals. You can use
# this feature to simplify the code (eliminate some temporary
# variables in this case) while still preserving code readability.
today = datetime.today().strftime('%Y-%m-%d')
registration_date = today,
active = 1,
sign_in_count = 2,
current_sign_in_on = today,
last_sign_in_on = today,
User.__init__() method has a large number of required parameters. Perhaps
your use case demands that, but you might give some thought to which of the
attributes are truly required in all usages. For example, the method
could default to using
registration_date if the two sign-on dates
are not provided (as we do above in
Regardless of that decision, if
testing and debugging make direct interaction with
__init__() tedious, you
can add more conveniences to the class, again using the
Managing user passwords is tricky business, at least in the real world. It’s
good that you store a hashed password rather than the plaintext, but I would
not include even the hashed password attribute in any kind of printing,
logging, etc. Adjust your
User.__str__() method accordingly.
UserRepository.add_user() you can simplify things. If you are testing for
truth, just do it directly (
if user.active) rather than indirectly (
if user.active == True). Also,
bool values in Python are a subclass of
199 + True == 200), so you can delete the conditional logic entirely and
just do something like this:
def add_user(self, user):
# Long lines can be made more readable by formatting
# them in the style of pretty-printed JSON. Notice how
# easy this style is to read, scan visually, and edit flexibly.
sql = '''
INSERT INTO users (
VALUES (%s, %s, %s, %s, %s, %s)
cursor = self.conn.cursor()
Your capitalization of the
User variable within various methods is
inconsistent both with your own usages and with the norms in the wider
FooBar for class names and
foo_bar for instance
variables is the most common approach.
DB operations can fail. If this is just an early learning exercise, you
can safely ignore the problem, but if your goal is to make the application
more robust, the operations should be wrapped in
as hinted at in your code comments.
In my experience, web applications like this will have various classes
representing the entities of interest (User, Book, Author, whatever). And
several of those classes will need DB operations: create, read, update, delete,
etc. And those DB operations will be fairly general, in the sense that if you
write a good method to update one kind of instance (eg, User), it can often be
made to do the same thing for other kinds of instances, provided that the
substantive classes (User, Book, etc) have attributes, properties, or methods
to deliver the information (things like table name and a list of
(COL_NAME, ATTRIBUTE_VALUE) tuples) that a general-purpose DB method needs in order to
perform its operation. All of which is to say that
be too narrow of a focus. Instead, think about whether and how you might
convert that class into just
If this web application is intended to be a long-running application, you’ll
want to wire it up to use the
logging framework at some point (rather than
just printing stuff), but that can come whenever you are ready to learn about
Regarding your question about frequency of saving DB changes, don’t worry about
it or overthink it. Build your application with reasonable Python code — you
are off to a good start — and if your application ever needs to achieve higher
scale, solve the problem when it is clearly visible on the horizon. It’s often
easier to change DBs, scale outward (more running instances of your application
in different processes or on different servers), or alter the data schema than
it is to write tricky “save every N operations” code.