Skip to content

Usethisone codereview1 pinchojd#9

Open
J14P wants to merge 5 commits intomainfrom
USETHISONE_Codereview1_pinchojd
Open

Usethisone codereview1 pinchojd#9
J14P wants to merge 5 commits intomainfrom
USETHISONE_Codereview1_pinchojd

Conversation

@J14P
Copy link
Copy Markdown
Collaborator

@J14P J14P commented Mar 5, 2023

Program Analysis: This app seems to be a workout app for users to track their progress. From what I can tell the users are able to select between push-ups, bench press, and cardio.

Was the program available on time? When I first attempted to do my code review on Friday, March 3rd, there wasn't any additional work completed besides creating the project. When I check on Sunday, March 5th there was some code that I was able to review. I would give them 50% effort for the code being available on time.

Does the program compile? It does

Is the program understandable? As far as the code they have written, I am able to understand the code once I took a deeper look into it because there aren't very many comments explaining what the purpose of their code is.

Rationale behind changes:
I made changes to the manifest.xml file to allow internet permissions so once the group wants to begin working with the database they won't have to deal with the internet permission not being there. I also decided to have android studio upgrade the plugins/kotlin to the latest updates because it is better to have the newest editions possible (as long as you can get them to compile).

Links to my commits:
https://github.com/aBock4/SunDial/commit/26ec0d074bb4765e22b1e3559de22ce2501ae26b
https://github.com/aBock4/SunDial/commit/c1ca40fbcba32e6aaa4abc649f6acd96da97cb2c

Three concepts I learned:

  1. It is important to comment your code so that it is easier for other people to understand your program.
  2. Trying to find the versions of all the different plugins that will run with each other can be very frustrating and time-consuming, but can be beneficial.
  3. How to add custom fonts into android studio and apply them to different UI tools.


object RetrofitClientInstance {
private var retrofit: Retrofit? = null
private val BASE_URL = ""
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an incomplete feature.
Are you using these anywhere?

Recommendation: if not used, remove it, or keep it in a separate development branch until it is complete. Keep the codebase as small as possible to do the job you want to do. Unused code is clutter, and bad documentation, which increases technical debt.
Only complete features, which meet the team's Definition of Done, should be merged to Main/Master.

import retrofit2.http.GET

interface IWorkoutRecDAO {
@GET("")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an incomplete feature.
Are you using these anywhere?

Recommendation: if not used, remove it, or keep it in a separate development branch until it is complete. Keep the codebase as small as possible to do the job you want to do. Unused code is clutter, and bad documentation, which increases technical debt.
Only complete features, which meet the team's Definition of Done, should be merged to Main/Master.

@discospiff
Copy link
Copy Markdown

I don't recommend merging this branch in its current state.

  • It's hard to determine what changed... but I believe part of that is due to the group not merging their development branches to main.
  • From the write up, it appears that the changes are not decreasing technical debt of existing code, but rather adding partially complete features and/or upgrading the build.gradle with dependencies that are not being used. Code review should focus on reducing technical debt of existing code, not adding new, possibly incomplete features. Incomplete features are technical debt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants