Negroncj code review1 #9
Open
Conversation
This file contains 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
added 6 commits
March 8, 2022 08:41
1. Took out dispachers.io from the test as it was just doing the same in university service 2. Moved the IUniversityService to dao and it was in the service folder 3. Made a variable named thread and put it in newSingleThreadContext to avoid technical debt Things I learned 1. @before and @after that resets the dispatcher to an original form 2. Review runTest to force the program to run the suspend function in the whenServiceDataAreReadAndParsed 3. That assert* helps a lot with data testing and makes it readable.
…rint_1_Code_Review
1. Took out dispachers.io from the test as it was just doing the same in university service 2. Moved the IUniversityService to dao and it was in the service folder 3. Made a variable named thread and put it in newSingleThreadContext to avoid technical debt Things I learned 1. @before and @after that resets the dispatcher to an original form 2. Review runTest to force the program to run the suspend function in the whenServiceDataAreReadAndParsed 3. That assert* helps a lot with data testing and makes it readable. Testing circleCI
Technical suggestions 1. Took out dispachers.io from the test as it was just doing the same in university service 2. Moved the IUniversityService to dao and it was in the service folder 3. Made a variable named thread and put it in newSingleThreadContext to avoid technical debt. This idea was brought in class in 3/8/2022 by the professor. I will have to apply it as well. Analysis of the Program -I will say seeing a retrofit work other than the plantplaces really gives me hope with our project as we are still stuck in our open weatherAPI Was the program available in UC Github on time? -Yes a working main was made on time is the program documented/commented well enough for you to understand? -yes, I was able to navigate around the code and quickly understand with the variables being clear and the read me gives me a idea of what the final project might be. Does the program Compile? - Yes in the test area there is a few logic tests and a JSON test that pass well. Even CircleCI checks with the tests so the team can know that they will pass or something is wrong with the project. Rationale to my changes - It was hard for me to do much as they just started working on it. I would say it was just some fixing that would help them get to the goal faster or simply make it easy for them in the future. I may have done a comments on how they did the retrofit as I would like to use there example on my project in the future. IN the end this is a solid start Links to three commits that I have made. 1. JoshAnness/Checkpoint@ab5101c 2. JoshAnness/Checkpoint@652e9df 3. JoshAnness/Checkpoint@d6e3ca5 Things I learned 1. @before and @after that resets the dispatcher to an original form 2. Review runTest to force the program to run the suspend function in the whenServiceDataAreReadAndParsed 3. That assert* helps a lot with data testing and makes it readable. Testing circleCI
Technical suggestions 1. Took out dispachers.io from the test as it was just doing the same in university service 2. Moved the IUniversityService to dao and it was in the service folder 3. Made a variable named thread and put it in newSingleThreadContext to avoid technical debt. This idea was brought in class in 3/8/2022 by the professor. I will have to apply it as well. Analysis of the Program -I will say seeing a retrofit work other than the plantplaces really gives me hope with our project as we are still stuck in our open weatherAPI Was the program available in UC Github on time? -Yes a working main was made on time is the program documented/commented well enough for you to understand? -yes, I was able to navigate around the code and quickly understand with the variables being clear and the read me gives me a idea of what the final project might be. Does the program Compile? - Yes in the test area there is a few logic tests and a JSON test that pass well. Even CircleCI checks with the tests so the team can know that they will pass or something is wrong with the project. Rationale to my changes - It was hard for me to do much as they just started working on it. I would say it was just some fixing that would help them get to the goal faster or simply make it easy for them in the future. I may have done a comments on how they did the retrofit as I would like to use there example on my project in the future. IN the end this is a solid start Links to three commits that I have made. 1. JoshAnness/Checkpoint@ab5101c 2. JoshAnness/Checkpoint@652e9df 3. JoshAnness/Checkpoint@d6e3ca5 Things I learned 1. @before and @after that resets the dispatcher to an original form 2. Review runTest to force the program to run the suspend function in the whenServiceDataAreReadAndParsed 3. That assert* helps a lot with data testing and makes it readable. Testing circleCI
Technical suggestions 1. Took out dispachers.io from the test as it was just doing the same in university service 2. Moved the IUniversityService to dao and it was in the service folder 3. Made a variable named thread and put it in newSingleThreadContext to avoid technical debt. This idea was brought in class in 3/8/2022 by the professor. I will have to apply it as well. Analysis of the Program -I will say seeing a retrofit work other than the plantplaces really gives me hope with our project as we are still stuck in our open weatherAPI Was the program available in UC Github on time? -Yes a working main was made on time is the program documented/commented well enough for you to understand? -yes, I was able to navigate around the code and quickly understand with the variables being clear and the read me gives me a idea of what the final project might be. Does the program Compile? - Yes in the test area there is a few logic tests and a JSON test that pass well. Even CircleCI checks with the tests so the team can know that they will pass or something is wrong with the project. Rationale to my changes - It was hard for me to do much as they just started working on it. I would say it was just some fixing that would help them get to the goal faster or simply make it easy for them in the future. I may have done a comments on how they did the retrofit as I would like to use there example on my project in the future. IN the end this is a solid start Links to three commits that I have made. 1. JoshAnness/Checkpoint@ab5101c 2. JoshAnness/Checkpoint@652e9df 3. JoshAnness/Checkpoint@d6e3ca5 Things I learned 1. @before and @after that resets the dispatcher to an original form 2. Review runTest to force the program to run the suspend function in the whenServiceDataAreReadAndParsed 3. That assert* helps a lot with data testing and makes it readable. Testing circleCI
discospiff
reviewed
Mar 10, 2022
Comment on lines
+11
to
+12
| //Saw this in the service folder. So I moved the file here. Will this be used in the future? | ||
| //If not just remove it as it seems like IUniversityDAO got you covered |
There was a problem hiding this comment.
Big no-no: do not put conversation in source code comments. That's clutter. Put the conversation here, in the Files Changed section of the pull request, where you can have a dialog with other developers.
discospiff
reviewed
Mar 10, 2022
| @@ -1,4 +1,4 @@ | |||
| package com.example.uniconnect.service | |||
| package com.example.uniconnect.dao | |||
There was a problem hiding this comment.
Since this is named IUniversityService, it almost definitely belongs in a service package, not a DAO package.
discospiff
reviewed
Mar 10, 2022
| @@ -29,16 +27,16 @@ import java.util.concurrent.TimeUnit | |||
| class UniConnectUnitTest { | |||
|
|
|||
| //lateinit var mvm : MainViewModel | |||
|
|
|||
| // | |||
discospiff
reviewed
Mar 10, 2022
Comment on lines
31
to
32
| //@MockK | ||
| //lateinit var mockUniService : UniversityService |
There was a problem hiding this comment.
While you're here, remove the commented out code.
Avoid pushing commented code to Version Control. Version control has tools that makes this unnecessary.
- You can look at history to see what used to be there.
- You can use branches for experimental features.
discospiff
reviewed
Mar 10, 2022
Comment on lines
+38
to
+39
| val thread = "UI thread" | ||
| private val mainThreadSurrogate = newSingleThreadContext(thread) |
There was a problem hiding this comment.
This doesn't appear to add any value... over creating a new line of code... unless the variable thread is used elsewhere.
discospiff
reviewed
Mar 10, 2022
Comment on lines
+75
to
+77
| //Removed the dispatchers IO that was here as it was doing the same in your service folder. | ||
| //Seems like it still runs great | ||
| //I am going to add the paths the program does on this as a example of documentation for debug in the future |
There was a problem hiding this comment.
Big no-no: do not put conversation in source code comments. That's clutter. Put the conversation here, in the Files Changed section of the pull request, where you can have a dialog with other developers.
discospiff
reviewed
Mar 10, 2022
Comment on lines
+90
to
+92
| //What this does is it goes to UniversityService with RetrofitClientService | ||
| //starts up the dao/IUniversityDAO and takes JSON data in the dto/University class and returns it to service/UniversityService | ||
| //gets the return of the list in all universities |
There was a problem hiding this comment.
If you're putting this much into a comment, make it a KDoc comment and put it on top of the method.
But... really... is this comment adding anything over and above what a developer could figure out by reading the code or running it in the debugger? If not, it's probably not needed... and thus, clutter.
|
I don't see enough technical debt reduction to merge this branch. I'd remove it without merging. |
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.
Code Review
Technical suggestions
1. Took out dispachers.io from the test as it was just doing the same in university service
2. Moved the IUniversityService to dao and it was in the service folder
3. Made a variable named thread and put it in newSingleThreadContext to avoid technical debt. This idea was brought in class in 3/8/2022 by the professor. I will have to apply it as well.
Analysis of the Program
-I will say seeing a retrofit work other than the plantplaces really gives me hope with our project as we are still stuck in our open weatherAPI
Was the program available in UC Github on time?
-Yes a working main was made on time
is the program documented/commented well enough for you to understand?
-yes, I was able to navigate around the code and quickly understand with the variables being clear and the read me gives me a idea of what the final project might be.
Does the program Compile?
Rationale to my changes
Links to three commits that I have made in my project.
Things I learned
1. @before and @after that resets the dispatcher to an original form
2. Review runTest to force the program to run the suspend function in the whenServiceDataAreReadAndParsed
3. That assert* helps a lot with data testing and makes it readable.