Open
Conversation
robin3a5
commented
Mar 4, 2023
| ) { | ||
| Column { | ||
| Text(text = "Height", fontWeight = FontWeight.Bold) | ||
| Text(text = stringResource(R.string.height), fontWeight = FontWeight.Bold) |
Collaborator
Author
There was a problem hiding this comment.
Using strings.xml is best practice and gets app ready for internationalization- https://developer.android.com/jetpack/compose/text
robin3a5
commented
Mar 4, 2023
| ) | ||
| ) | ||
| val goals = listOf( | ||
| var goals = listOf( |
Collaborator
Author
There was a problem hiding this comment.
Ignore this, read the doc wrong
robin3a5
commented
Mar 4, 2023
| goals = goals, | ||
| workouts = workouts | ||
| ) | ||
| for (workout in workouts) { |
Collaborator
Author
There was a problem hiding this comment.
Pulling this out of your compose functions improves performance since this will never have to be recomposed - https://developer.android.com/jetpack/compose/performance/bestpractices
robin3a5
commented
Mar 4, 2023
| import androidx.compose.ui.unit.dp | ||
| import com.example.workoutapp.ui.theme.WorkoutAppTheme | ||
|
|
||
| const val PADDING_16 = 16 |
Collaborator
Author
There was a problem hiding this comment.
Added some constants of reused numbers to avoid some magic numbers in code - https://en.wikipedia.org/wiki/Magic_number_(programming)
|
Several good concepts to harvest.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Analysis of the program: I only examined the profile page part of the program, but it looks great. I like the use of the loading bar to display progress on goals and the expand and collapse of workouts is very nice.
Was the program available in UC Github on time? Yes
Is the program documented/commented well enough for you to understand? Yes
Does the program compile? Yes
Rationale behind your changes:
For internationalization any string constants in the app should be placed in strings.xml - https://developer.android.com/jetpack/compose/text
Its best to put computations outside of composable functions since they could recompose often hurting performance -https://developer.android.com/jetpack/compose/performance/bestpractices
Best not to have some "Magic numbers" in your code especially those that are used repeatedly -
Don't have an exact resource on it, just known as a best practice - https://en.wikipedia.org/wiki/Magic_number_(programming)
Links to three specific commits that you made to your own group's GitHub repository before the end of sprint deadline:
https://github.com/steelesh/IdeaStorm/commit/6b05db39cfc05e081c726bb7f9d3254c600ace7d
https://github.com/steelesh/IdeaStorm/commit/98ba5fff37076a2e04f08c8f5c6bfdb64290c50c
https://github.com/steelesh/IdeaStorm/commit/c192d7a5585b1e139cd5dc3fe917e05fa4353f9d
Specific Technical Concepts:
How to use stringResource to place strings from strings.xml into composable functions
How to avoid expensive computations in composable functions
How to add padding to columns for proper alignment of elements