Dragonfly-Sno Ochoa #41
Conversation
There was a problem hiding this comment.
Sno, I would like you to resubmit your Task List project. In the comments you will find what you need to refactor (is_complete). Once you've resubmitted, message me via Slack DM and I can take another look.
There are also a lot of places where you could benefit from performing refactors. I strongly encourage you to read the comments I left and do so. Please feel free to reach out if you have any questions.
app/models/goal.py
Outdated
|
|
||
| class Goal(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] = mapped_column(db.String, nullable=False) |
There was a problem hiding this comment.
SQLAlchemy uses declarative typing annotation (title: Mapped[str]). From this line of code it is able to create the respective column in the database. This excerpt is from the SQLAlchemy documentation:
"...Columns that are part of the table are declared, by adding attributes that include a special typing annotation called Mapped. The name of each attribute corresponds to the column that is to be part of the database table. The datatype of each column is taken first from the Python datatype that’s associated with each Mapped annotation; int for INTEGER, str for VARCHAR, etc. Nullability derives from whether or not the Optional[] type modifier is used. More specific typing information may be indicated using SQLAlchemy type objects in the right side mapped_column() directive..."
Meaning that we only need to use mapped_column for more specific things like setting ForeignKey, auto-incrementing, etc. Since you aren't leveraging that (all the parameters you passed in are handled by the type annotation), it is safe to remove.
app/routes/task_routes.py
Outdated
| query = db.select(Task) | ||
| tasks = db.session.execute(query).scalars().all() | ||
|
|
||
| sort_order_param = request.args.get("sort", "asc") | ||
| if sort_order_param == "desc": | ||
| query = query.order_by(Task.title.desc()) | ||
| else: | ||
| query = query.order_by(Task.title.asc()) | ||
|
|
||
| title_param = request.args.get("title") | ||
| if title_param: | ||
| query = query.where(Task.title.ilike(f"%{title_param}%")) | ||
|
|
||
| description_param = request.args.get("description") | ||
| if description_param: | ||
| query = query.where(Task.description.ilike(f"%{description_param}%")) | ||
|
|
||
| completed_at_param =request.args.get("completed_at") | ||
| if completed_at_param: | ||
| query = query.where(Task.completed_at.ilike(f"%{completed_at_param}%")) | ||
|
|
||
| is_complete_param = request.args.get("is_complete") | ||
| if is_complete_param: | ||
| # is_complete_bool = is_complete_param.lower() == "true" | ||
| # query = query.where(Task.is_complete == is_complete_bool) | ||
|
|
||
| if is_complete_param.lower() == "true": | ||
| query = query.where(Task.task_completed == True) | ||
| elif is_complete_param.lower() == "false": | ||
| query = query.where(Task.task_completed == False) | ||
|
|
||
| query = query.order_by(Task.id) | ||
| tasks = db.session.scalars(query) |
There was a problem hiding this comment.
I would say that this a great opportunity for a refactor. Note how it takes up most of the function logic. We lose sight of the function's purpose, handling the a request to get all tasks. Right now, it seems like its purpose is to build a query based off of certain filters.
app/routes/task_routes.py
Outdated
| slack_token = os.environ.get("SLACKBOT_API_TOKEN") | ||
| slack_channel = os.getenv("SLACK_CHANNEL", "#sno-task-tracker-bot2") | ||
| slack_text = f"Someone just completed the task: *{task.title}* ! 🎉" | ||
|
|
||
| if slack_token: | ||
| url = "https://slack.com/api/chat.postMessage" | ||
| headers = { | ||
| "Authorization": f"Bearer {slack_token}", | ||
| "Content-Type": "application/json" | ||
| } | ||
| slack_data = { | ||
| "channel": slack_channel, | ||
| "text": slack_text | ||
| } | ||
| slack_response = requests.post(url, headers=headers, json=slack_data) | ||
|
|
||
| if not slack_response.ok: | ||
| print("Slack error:", slack_response.text) |
There was a problem hiding this comment.
Nice work getting Slack integration in place! A suggestion for improving readability and maintaining Single Responsibility Principle (SRP) is to move the Slack API call into a separate helper function (e.g., send_slack_notification). This keeps the route function focused purely on request processing and database logic, while isolating external service communication. It’ll also make the Slack functionality easier to reuse and test in the future.
Here's an example of what that helper could look like:
def send_slack_notification(message):
slack_token = os.environ.get("SLACK_TOKEN")
chanel = os.environ.get("SLACK_CHANEL_ID")
headers = {
"Authorization": f"Bearer {slack_token}"
}
data = {
"channel": channel,
"text": message
}
response = requests.post("https://slack.com/api/chat.postMessage", headers=headers, json=data)
app/routes/task_routes.py
Outdated
| task.title = request_body["title"] | ||
| task.description = request_body["description"] |
There was a problem hiding this comment.
This is another opportunity to D.R.Y. up our code! Notice how in this route and the PUT route for goal_routes.py we follow the pattern of:
object.ATTRIBUTE = request_body["ATTRIBUTE"]We use hasattr and setattr to make a helper function to update our Task and Goal model. It would look like this:
def update_model(obj, data):
for attr, value in data.items():
if hasattr(obj, attr):
setattr(obj, attr, value)
db.session.commit()
return Response(status=204, mimetype="application/json")This refactor not only makes our code D.R.Y but shows that we recognize logic that has higher level usability while handling cases of keys not being found!
| # assertion 2 goes here | ||
| # assertion 3 goes here | ||
| # ---- Complete Assertions Here ---- | ||
| assert response.status_code == 204 |
There was a problem hiding this comment.
@ra-choa, you should also check the database to ensure that your changes persisted.
Co-authored-by: Mikelle <119449245+mikellewade@users.noreply.github.com>
Co-authored-by: Mikelle <119449245+mikellewade@users.noreply.github.com>
Co-authored-by: Mikelle <119449245+mikellewade@users.noreply.github.com>
2 participants
No description provided.