Repository navigation
Coding Tracker code review - #296
Open
Kunikazu723 wants to merge 93 commits into
Open
Kunikazu723 wants to merge 93 commits into
Kunikazu723 wants to merge 93 commits into
Conversation
Create and use the Configuration package provided by Microsoft to handle configuration properties effectively (I don't really know why I should not use a simple JSON desserializer)
UserInput is a dependency of UserInterface, the latter being public. Allowing access of an internal member in a public member breaks the contract. So it needs to be corrected by either reducing UserInterface's access modifer permission or elevating UserInput's access modifier. The last one being the choice I made
I know I should focus on making atomic commits. But now I'm too burned out for it and for that I am sorry.
The user must input a valid end and start date. So the need of a method that can inform if a start date happened earlier than an end date emerges.
…ngle line for better modularity and debugging
Since the DAO is now handled by the UserInterface, having it at the Service is wasteful. This ensures that the Service only handles logic and nothing more.
Service is too broad and can mean a lot of things depending on which source you base it on. Since the only thing the class is performing now is the build of a readable table(consequence of letting the UI talk directly to the DAO), naming it so makes it clear to what it actually does.
…r not This is a neat way to add the new stopwatch funcitonality since it follows the same pattern as GetStartEndDate manually. This glues them toghether a little bit but it is readable and a nice way to solve it
… inserted a value that already exists in the database
… feedback into what the user is currently inserting
…s in client code allowing for default ISO format to be stored Previously the date stored in the database followed the date format defined in the appsetting.json file. This brought many limitations and akward code, for example: Changing dateFormat in appsettings after the seeded database would make the database store more than one format; TryParseExact already offers an out variable that can be returned directly at UserInput.GetDateTime, no need to make it a string and then parse it as datetime later on. This implementation also takes advantage of System.Data.Sqlite driver that maps DateTime objects directly to ISO standard TEXT types when storing it into the database. Overall this makes the code cleaner and avoid breaking default storage format
Not an atomic commit I know. Unfortunately I fell in this rabbit hole after midnight and I'm kind of burned out. Sorry
…to type select the Order in which the filter should be displayed
System.Data.Sqlite.Core is an older legacy driver. It is advisable and good practice to use the newer Microsof.Data.Sqlite driver
🚀 Project Submitted!Thanks for submitting your project! I will review it soon. 📋 Please check the dashboard to make sure the project is marked as Pending. |
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
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.
Cool project, learned some nice valuable lessons with this one. (Dapper is pretty neat). Thank you for the guidance