Conversation
chrisjamiecarter
left a comment
chrisjamiecarter
left a comment
There was a problem hiding this comment.
Hey @jollejonas 👋,
Excellent work on your ExerciseTracker project submission 🎉!
I have performed a peer review and have added some comments. Review/ignore as you wish.
🟠 Needs better validation handling, you can input an endTime less than the startTime, but when you do, it fails to save, and then the app becomes unusable. i.e., subsequent valid entry will also fail to be added.
I suggest revisiting to try understand and fix any issues 🔧 , but as these do not relate to the requirements they will not block my approval.
Feel free to reach out if you have any questions or if you'd like to collaborate further on any of these points 🆘 .
I will go ahead and mark as approved, keep up the excellent work on the next projects! 😊
Best regards,
@chrisjamiecarter 👍
| using ExerciseTracker.jollejonas.Models; | ||
|
|
||
| namespace ExerciseTracker.jollejonas.Controllers; | ||
| public class ExerciseController |
There was a problem hiding this comment.
🟢 Clean Controller
⭐ Nice, clean and lightweight!
|
|
||
| protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) | ||
| { | ||
| optionsBuilder.UseSqlServer("Server = (localdb)\\mssqllocaldb; Database = ExerciseTrackerContext; Trusted_Connection = True; MultipleActiveResultSets = true"); |
There was a problem hiding this comment.
🟠 AppSettings
💡 ConnectionStrings should be stored in appsettings or app.config and also retrieved from there. It seems like you have an appsettings, and a connectionstring value, but are just not using it.
| } | ||
| }, | ||
| "ConnectionStrings": { | ||
| "PhonebookContext": "Server=(localdb)\\mssqllocaldb;Database=ExerciseTrackerContext;Trusted_Connection=True;MultipleActiveResultSets=true" |
| using ExerciseTracker.jollejonas.Enums; | ||
| using ExerciseTracker.jollejonas.Repositories; | ||
|
|
||
| class Program |
There was a problem hiding this comment.
❓ No README of how to use your app
|
|
||
| public enum MenuOptions | ||
| { | ||
| AddExercise = 1, |
|
|
||
| namespace ExerciseTracker.jollejonas.Repositories; | ||
|
|
||
| public class ExerciseRepository : IExerciseRepository |
There was a problem hiding this comment.
🟢 Clean Repository
⭐ Nice, clean and lightweight repository
| { | ||
| if (id == 0) | ||
| { | ||
| throw new ArgumentNullException(); |
There was a problem hiding this comment.
❓ id == 0 does not equal id == null... Should this not be ArgumentOutOfRangeException or something more relevant.
| } | ||
| if (!_context.Exercises.Any(e => e.Id == id)) | ||
| { | ||
| throw new KeyNotFoundException(); |
There was a problem hiding this comment.
🟡 Improve Logic
💡 You could have made the return type of this method a nullable Exercise, i.e., public Exercise? GetExerciseById(int id) { ... }
Then here you could have done return null; or better yet, exclude this all together, as your line below return _context.Exercises.FirstOrDefault(e => e.Id == id); will return null if the ID does not exist.
2 participants
Project finished. Looking forward to your review! 😄