Closed raffaelfoidl closed 2 years ago
In GitLab by @11712616 on Feb 4, 2022, 19:15
requested review from @11775820
In GitLab by @11712616 on Feb 4, 2022, 19:26
Commented on server/src/test/java/com/witness/server/integration/web/WorkoutLogControllerTest.java line 115
my nemesis
In GitLab by @11712616 on Feb 5, 2022, 21:13
added 1 commit
In GitLab by @11712616 on Feb 5, 2022, 21:27
added 7 commits
develop
In GitLab by @11712616 on Feb 6, 2022, 14:27
added 7 commits
develop
In GitLab by @11712616 on Feb 7, 2022, 17:56
added 1 commit
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/widgets/exercises/exercises_by_muscle_group_screen.dart line 35
Why can't we just use widget._muscleGroup
and _widget._selectExerciseAction
. Aren't such variables (in the whole client codebase) very redundant?
I know that I, absentminded, did something like this more than once. But now that my thoughts are clear again, I think it's just useless to introduce such variables? Or do I miss some benefits or caveats for doing one thing over the other (apart from redundant lines of code)?
Of course, there are cases where we cannot do this (where you ingeniously introduced didUpdateWidget
, i.e. the variables are not final
).
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/extensions/date_time_extensions.dart line 47
How about moving this to a new extension DateTimeExtensions
in the same class and renaming the existing one to TZDateTimeExtensions
(would be more fitting, anyway)? Then we could convert it to a regular extension method (not a static
one) and significantly shorten the invocations (DateTimeExtensions.onlyDateFromDateTime(date)
become date.onlyDateFromDateTime()
). Optionally, we could also choose a better name, e.g. onlyTZDate
or something like that - but that's up to you.
I have attached a patch including the changes introduced by my suggestion. Note that is also restructures the tests accordingly (an own group per extension
), that gives nicer CI and IDE outputs (proper nesting etc.). I have also done that for number_extensions
where we failed to do so (and I have also renamed it to its correct name number_extensions_test.dart
.
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/localization/app_en.arb line 945
"The start date must not ... end date."?
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/models/workouts/set_log_create.dart line 19
Since we are in the SetLogCreate
class and we are returning a SetLogCreate
instance (i.e. this is a simple static factory method), it would imo be a bit more apt to call this method fromSetLog
.
I would go even further and turn this static factory method into a factory constructor:
factory SetLogCreate.fromSetLog(final SetLog setLog) {
if (setLog is RepsSetLog) {
return RepsSetLogCreate(rpe: setLog.rpe, weightG: setLog.weightG, resistanceBands: setLog.resistanceBands, reps: setLog.reps);
} else if (setLog is TimeSetLog) {
return TimeSetLogCreate(rpe: setLog.rpe, weightG: setLog.weightG, resistanceBands: setLog.resistanceBands, seconds: setLog.seconds);
}
throw Exception('Could not identify type of set log!');
}
Then, we can make the invocation of this constructor more succinct by employing a method reference (in exercise_log_create
):
ExerciseLogCreate.fromExerciseLog(final ExerciseLog exerciseLog)
: this(
exerciseId: exerciseLog.exercise.id,
comment: exerciseLog.comment,
setLogs: exerciseLog.setLogs.map(SetLogCreate.fromSetLog).toList(), // here
);
(technically speaking, that should have already been possible before, but the factory constructor is just nice syntactic sugar we should use here)
The spoiler below contains yet again a patch with the changes described above and other occurrences where we could have used a method reference, but I had missed it.
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/models/workouts/set_log_create_converter.dart line 29
@JsonSerializable()
annotation to the example (as well as to the examples of other converters, where applicable), missed that the last time.models/common
package to models/converter(s)
and move set_log_create_converter.dart
as well as set_log_converter.dart
to that package. What do you think?In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/providers/workout_log_provider.dart line 32
I'd maybe call this variable _workoutLogsCount
or workoutLogsCountByDay
? For me, the fact that it only contains non-empty workout logs is a server-side implementation detail and not that relevant to the client, let alone this specific provider. It does not make any difference for the implementation of the provider and its consumers, therefore I'd simply drop it. If you really want an indication of this fact, then I suggest amending the log message in getLoggingDaysInPeriod
accordingly.
Do you agree?
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/providers/workout_log_provider.dart line 26
I think it's time to fix something that has been bothering me for quite some time now. Those ....?... ?? <...., .....>{}
constructs are a bit annoying. Thanks to Dart's great generics support, I'd like to introduce extension methods on nullable maps and lists so that we can simply call .orEmtpy()
on those nullable maps and lists.
See patch below for required changes, tests included. You may choose a better name if you want.
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/providers/workout_log_provider.dart line 75
How about using our whereKeys
extension method (as well as expression body)?
final datesNotInResponse = _nonEmptyWorkoutLogsCount.whereKeys(
(final date) => date.key.compareTo(startDate) >= 0 && date.key.compareTo(endDate) <= 0 && !resultMap!.keys.contains(date.key),
);
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/providers/workout_log_provider.dart line 85
date
, entry
(no underscore for local variables)
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/providers/workout_log_provider.dart line 170
Wouldn't logCreates
be better instead of logsCreate
?
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/providers/workout_log_provider.dart line 142
new exercise logs (both in success and error case)
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/widgets/common/dialog_helper.dart line 115
MediaQuery.of(context)
only once at the beginning of the method and reuse it, also for "nested" method calls.return Column()
lambda to an expression body => Column()
.copyWith(textScaleFactor: textScaleFactor)
(no trailing comma) and reformat),
after the closing parenthesis of Column()
, pls reformat.BuildContext context
parameter of the Builder
's builder
property to something like innerContext
or the like. Currently, we are shadowing the getDatePicker
method's parameter name context
, which should always be avoided because it may lead to confusing behaviour.Today
button which jumps to the current date? Or is this not part of the standard Material Design Flutter DatePicker?Apart from that, why the sudden interest in adaptability and responsiveness? It's really nice that you've done it here, but we've never put much attention to those aspects yet. Is there a specific reason?
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/widgets/common/dialog_helper.dart line 144
Get MediaQuery
from other method.
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/widgets/workouts/workout_log_screen.dart line 38
_logger.v(prepare('_createNewWorkout()'));
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/widgets/workouts/workout_log_screen.dart line 57
Please comma after exerciseLogs
parameter and reformat (also applies to some other methods in this file (and possibly also provider).
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/widgets/workouts/workout_log_screen.dart line 68
What happens if the fetchWorkoutLogsByDay
call produces an error in the backend? Is this handled gracefully in the client with a respective error message? It does not necessarily seem so, I can't see any error handling.
Can you give me a short rundown of your considerations in regards to client-side error handling in this MR? Did I just stumble upon the only two mishaps or are there potentially more instances where server-side errors are not handled gracefully in the client?
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/widgets/workouts/workout_log_screen.dart line 125
Please no comma after child
and reformat.
This method should get a doc comment or so explaining what it actually does. buildRefreshableWidget
alone is not really descriptive without context.
Apart from that, wouldn't the more "natural" or "correct" extraction be that you have child: child
and the parameter is the Text
wrapped in Center
, just as before?
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/widgets/workouts/workout_log_screen.dart line 209
Why copy all of those properties from the Widget to the State? Can't you just access them with widget.------
where needed? This also applies to some other occurrences in the codebase. It's not always possible, though. For example, of course, for non-final variables where the widget.---
variable is just an initial value. For those cases where we utilize didUpdateWidget
, I'm not sure if we are allowed to tinker there (due to the if
we have there) without breaking anything.
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/widgets/workouts/workout_log_screen.dart line 29
_WorkoutFloatingActionButton
is still within the allowed range of lines of code. However, with about 220 LOC, WorkoutLogScreen
exceeds the maximum number of lines of code per widget (see Wiki) by around 100.
You are more familiar with the code, please see if you are able to extract one or another widget to make this screen more compact. This would not only increase readability and maintainability, but also performance because re-renderings don't need to re-create the whole huge widget tree of WorkoutLogScreen
every time.
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/widgets/workouts/copy_exercise_logs_dialog.dart line 35
So many final
literal copies. Why not use widget.----
verbatim?
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/widgets/workouts/copy_exercise_logs_dialog.dart line 191
Please remove map
's generic parameter List<ExerciseLog
, it's inferred by the machine and obvious to humans owing to the return type.
Maybe also add comment like // get list of list of exercise logs from all workout logs and flatten them into one single list
for people like me?
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/widgets/workouts/copy_exercise_logs_dialog.dart line 103
Pleas remove <Widget>
, everywhere if possible (my global search tells me 6 occurrences - one of them is, I think, invalid withtoutt the type arg).
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/widgets/workouts/copy_exercise_logs_dialog.dart line 116
One thing I have missed while playing around with big and numerous workout logs:
Would it be possible to include a button (toggle button or so) next to the Workout X
that selects all/no exercise logs of the specific workout log? Doesn't need to be sophisticated, i.e. no matter how many exercise logs are selected, first tap is select all, second is select none etc.
I don't know how much effort that is and how nicely it plays with the UI (size constraints) here. If you are able to do this in 15-20 min at max, I would greatly appreciate it. If it takes more than that, we'll leave it as is.
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/widgets/workouts/select_workout_log_dialog.dart line 15
Easily fits on one line. Also applies to some other new Widget constructors in this MR.
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/widgets/workouts/select_workout_log_dialog.dart line 43
Please no trailing ,
for Text
argument and reformat
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/widgets/workouts/select_workout_log_dialog.dart line 41
I found this screen quite confusing, to be honest. The fact that we have ListTile
s here (and, from a user perspective, "regular text with icons"), suggests that we might be able to select multiple workouts in this screen. One knows that from other apps where the +
turns into a checkmark after clicking the list item.
Maybe we could use something different than a ListTile, for example a simple ElevatedButton
with a more decent color? Just anything different from regular text so that users have a clear indication that they can tap one and only one of the options displayed (and that doesn't suggest that they can select multiple items).
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/test/unit/extensions/date_time_extensions_test.dart line 100
See comment in date_time_extensions.dart
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on client/lib/widgets/workouts/workout_log_screen.dart line 35
There is no error handling. All the calendar days stay disabled, which is UI-wise already the wisest option. However, we should strive not to have something like this (CONFIGURATION_NOT_FOUND
is the ServerError
I returned temporarily to produce the error):
Normally we have the provider calls that return Future<void>
in COnsumers, FutureBUilders and so on - so we have an AsyncSnapshot
the Future.error
is propagated to. In this case, we don't have that and, without looking it up, I don't know how to gracefully handle it in the _fetchLoggiingDaysInMonth
method. I hope there is an easy way you can find to do so. In any way, we should display the translated error to the user...I can't imagine that getting the error object of a failed Future
is too difficult (maybe we have to use then
instead of await
, I don't know - you'll hopefully find out).
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on server/src/main/java/com/witness/server/enumeration/ServerError.java line 211
More context please, both for the enum member name as well as its documentation.
The name-change is semi-mandatory, but the documentation must be changed. Name + docs have to be descriptive enough to unambiguously determine the use case for the error key. This is not the case here.
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on server/src/main/java/com/witness/server/repository/WorkoutLogRepository.java line 39
I would do it the other way round: Remove the NonEmtpy
from the method name (to make it more readable) and mention the documentation (in any case, it should be mentioned in the documentation) that only workout logs with at least one exercise log are returned.
Regarding the query itself, I'd rather hide the join and instead make use of HQL:
SELECT
w
FROM
WorkoutLog w
WHERE
w.exerciseLogs.size > 0 AND w.loggedOn >= :loggedOnStart AND w.loggedOn <= :loggedOnEnd AND w.user.firebaseId = :firebaseId
That way, in my opinion, the intention of the query becomes much clearer and more explicit. What do you think?
In GitLab by @11775820 on Feb 7, 2022, 23:01
Maybe I'm mistaken, but I don't see a test case where the criterion of non-emptiness is exercised. Would you mind duplicating the first test case, removing the exercise logs (setting it to []
) with an expectation of no returned IDs. Would that do the trick?
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on server/src/main/java/com/witness/server/repository/WorkoutLogRepository.java line 11
Please add docs to my method:
/**
* Queries all workout logs that logged between a given start date and a given end date by the user with the provided Firebase ID.
*
* @param loggedOnStart start date of the period that the resulting workout logs should be logged in
* @param loggedOnEnd end date of the period that the resulting workout logs should be logged in
* @param firebaseId Firebase ID of the user that logged the resulting workout logs
* @return list of {@link WorkoutLog} instances logged between {@code loggedOnStart} and {@code loggedOnEnd} by {@code firebaseId}
*/
List<WorkoutLog> findByLoggedOnBetweenAndUserFirebaseIdEquals(ZonedDateTime loggedOnStart, ZonedDateTime loggedOnEnd, String firebaseId);
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on server/src/main/java/com/witness/server/service/impl/WorkoutLogServiceImpl.java line 281
Please reverse the order of parameters to make invocations a little bit clearer:
startDate, endDate, queryExecution
Furthermore, rename queryExecution
to workoutLogGetter
or something better - queryExecution
is somehow weird :thinking:
Also, rename startDateWithoutTime
to startOfStartDay
and endDateWithoutTime
to endOfEndDay
or so. The with
calls do not truncate the time
part (which your naming suggests), but set them to the earliest and latest value possible, respectively. The variable naming should indicate that.
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on server/src/main/java/com/witness/server/service/impl/WorkoutLogServiceImpl.java line 77
..must lie before the end date
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on server/src/main/java/com/witness/server/service/impl/WorkoutLogServiceImpl.java line 82
Please use Collectors.groupingBy
(no static imports in production code unless justified, e.g. for readability - cf. OpenApiConfig.java
).
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on server/src/test/java/com/witness/server/integration/web/WorkoutLogControllerTest.java line 109
missing new line after region
In GitLab by @11775820 on Feb 7, 2022, 23:01
Commented on server/src/test/java/com/witness/server/integration/web/WorkoutLogControllerTest.java line 115
Suggestion (this also fixes two deprecation warnings from AssertJ). Obiously I didn't push it to test it in the CI, but I'm fairly confident that it works under any circumstance:
In GitLab by @11775820 on Feb 7, 2022, 23:01
Looks very nice. Well done :)
In GitLab by @11712616 on Feb 12, 2022, 12:43
added 1 commit
In GitLab by @11775820 on Feb 12, 2022, 13:09
added 1 commit
In GitLab by @11712616 on Feb 12, 2022, 13:24
Commented on client/lib/localization/app_en.arb line 945
changed this line in version 9 of the diff
In GitLab by @11712616 on Feb 12, 2022, 13:24
Commented on client/lib/widgets/workouts/copy_exercise_logs_dialog.dart line 35
changed this line in version 9 of the diff
In GitLab by @11712616 on Feb 12, 2022, 13:24
added 3 commits
In GitLab by @11712616 on Feb 12, 2022, 13:40
Commented on client/lib/models/workouts/set_log_create_converter.dart line 29
changed this line in version 10 of the diff
In GitLab by @11712616 on Feb 4, 2022, 19:15
Merges 40-copy-previously-logged-workout -> develop
Closes #40