Conversation
| lateinit var workoutService: WorkoutService() | ||
| lateinit var workoutService: WorkoutService |
There was a problem hiding this comment.
Correct. This should either be:
= WorkoutService() (constructor call) or
: WorkoutService (variable of type WorkoutService)
Since this is preceded with the lateinit modifier, it indicates that this should be a variable of type WorkoutService, and the code reviewer made the correct change.
To the group: I highly doubt this would have compiled as-is. Except for TDD unit tests, which should be in their own branch, code you commit to Version Control should always compile. I recommend you stop everything you are doing, and make sure your project builds, before proceeding on anything else. Otherwise, you're building a house on an instable foundation.
| @@ -47,7 +43,6 @@ class WorkoutUnitTest { | |||
| Dispatchers.setMain(mainThreadSurrogate) | |||
| MockKAnnotations.init(this) | |||
| gam = GroupsActivity() | |||
There was a problem hiding this comment.
Rename this to a descriptive variable name. Avoid acronyms and abbreviations, as they are not descriptive, and you won't remember what acronym/abbreviation you used later.
| "Continue the movement until your biceps are fully contracted and the bar is at shoulder level. Hold the contracted position for a second and squeeze the biceps hard.", | ||
| "Slowly begin to bring the bar back to starting position as your breathe in.", | ||
| "Repeat for the recommended amount of repetitions.", "strength") | ||
| val workout = Workout("Barbell Curl", "pull", "beginner", "isolation", "barbell", "biceps", "forearms", |
There was a problem hiding this comment.
I'd move this to strings.xml, or some other configuration file (like a JSON feed that you read.) A) Don't assume everyone speaks English. B) Hardcoded text means the entire application has to be recompiled, and a new version released, for a simple text change.
| "Repeat for the recommended amount of repetitions.", "strength") | ||
| val workout = Workout("Barbell Curl", "pull", "beginner", "isolation", "barbell", "biceps", "forearms", | ||
| "Example Workout Instructions.", "strength") | ||
| assertTrue(workout.ToString().equals("Barbell Curl pull beginner isolation barbell biceps forearms Stand up with your torso upright while holding a barbell at a shoulder-width grip. The palm of your hands should be facing forward and the elbows should be close to the torso. This will be your starting position. While holding the upper arms stationary, curl the weights forward while contracting the biceps as you breathe out. Tip: Only the forearms should move. Continue the movement until your biceps are fully contracted and the bar is at shoulder level. Hold the contracted position for a second and squeeze the biceps hard. Slowly begin to bring the bar back to starting position as your breathe in. Repeat for the recommended amount of repetitions. strength")) |
There was a problem hiding this comment.
Yikes. This much text in an assert is brittle! Change one word and your unit test fails, and pipeline breaks. Make this configurable, possibly with your own JSON feed.
| //The json service we are using has split json files we need to parse so this will likely change as | ||
| //we research how to make it work. | ||
| @Test | ||
| fun `Given service connects to Countries JSON stream when data are read and parsed then country collection should be greater than zero`() =runTest { |
There was a problem hiding this comment.
Good. Function name should be descriptive. "Countries" is a copy-paste artifact, and is misleading.
| coEvery {workoutService.fetchWorkouts()} returns workouts | ||
|
|
||
| gam.workoutService = mockworkoutService | ||
| gam.workoutService = mockWorkoutService |
There was a problem hiding this comment.
Good. Use proper capitalization conventions.
| allCountries = t | ||
| allWorkouts = t |
|
Several good suggestions here. I recommend the group merge this branch. I also recommend that the group is laser-focused on ensuring the code they have pushed to main compiles. That's order of business #1 for building an application with minimal defects. |
2 participants
Program Analysis:
Social Workout appears to be an application centered around tracking your workouts and exercises, as stated in their READ_ME.md. That being said, there wasn't much substance to their code to review and make helpful changes to. Their most robust branch was mostly just incomplete unit tests. I only went so far to fix these errors, as I didn't want to overstep my bounds of being a code reviewer.
Was the Program Available on Time?:
I would say no, as there wasn't much code to actually review within this project. However, the project team did a good job of staying in communicating about their code and adding me as a collaborator.
Is the Program Understandable?:
I can somewhat tell the direction that this group is going towards, but their code is extremely difficult to understand. From what I can tell, they're stuck trying to find a proper JSON data source before they continue further. Based on what I've seen, they might want to focus on finding an easily parsable data source or continue with a theoretical one, because they still don't have the core of their project developed yet and it would be worse to fall behind.
Does the Program Compile?:
No.
Rationale Behind Changes:
As I said before, there wasn't much to review and offer feedback on. I focused mostly on refactoring the names of several confusing variables and objects, some of which seemed as though they belonged in a different project. Another large change I made was compressing the exercise instructions down to take up less space. I also implemented some of the environments recommended changes, such as changing some declarations from var to val.
Links to Three Commits to My Group's Project:
https://github.com/gasawase/SunDial/commit/5c04e279df359eeb4dfb13a3bfee4ad89e306728
https://github.com/gasawase/SunDial/commit/6e3fa86b86615e9faf8c8786dafe05f86a29f97f
https://github.com/gasawase/SunDial/commit/f7801cd0b0c6a4784cd43d81c5e5874d2033efe7
Three Concepts I Learned:
1.) Read-only variables should always be declared using "val" instead of "var", as "var" is used for variables that can be reassigned. This is something to keep in mind when making your unit tests https://www.educative.io/answers/what-is-the-difference-between-var-val-and-const-in-kotlin.
2.) It's important to write your code to be readable and easily understandable by other developers. Huge blocks of text and oddly named variables make it more difficult for an outsider to interpret your code https://docs.sonarqube.org/latest/.
3.) It's important to keep a consistent naming schema for your project / application. I noticed that the project is called "Workout-App" in the VCS and "Social Workout" in the READ_ME.md file https://kotlinlang.org/docs/coding-conventions.html#property-names.