Peer Review: Big Pimpin' Blake Polidore#1
Open
blakepolidore wants to merge 1 commit intoadao1123:masterfrom
Open
Peer Review: Big Pimpin' Blake Polidore#1blakepolidore wants to merge 1 commit intoadao1123:masterfrom
blakepolidore wants to merge 1 commit intoadao1123:masterfrom
Conversation
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.
PEER REVIEW
Questions to answer:
_Score 3: I'm really impressed with what I'm seeing in this project. It definitely meets the requirements. You didn't need a service. I'm very curious to see some of these 3rd party libraries when you present. I love how simplified your code is for you activities and fragments. No wasted lines, very unlike my code. You click listeners in particular fascinate me. I'm impressed with all the custom clicking functionality you wrote. I want to add fling and swipe functionality to my recycler code and may steal your listeners. I guess you could've done sql lite but I think it makes way more sense to do firebase for live updates and have reviews public to all users. _
Is your peer making API calls, using SDK's/third-party libraries?
Is your peer making use of Services? If so, are they offloading long tasks to a separate thread, i.e. AsyncTask, Runnable, IntentService, etc.
Is your peer making use of Fragments? If so, are they passing data from Fragment to Activity via interfaces? If not, why did absense of Fragments make sense?
Is your peer making use of RecyclerView? If so, does it appear to be working correctly ( implementation and otherwise )?
Is your peer making use of some sort of persistent storage, i.e. Firebase or SQLite? If so, why do you think Firebase/SQLite was chosen? Could they have used one or the other instead and why?
2. Does the project appear to be creative, innovative, and different from any competition? Write up one sentence on your findings and give a score 0-3.
Score 3: I'm a big fan of how it looks. I'm amazed at how many recycler views you are incorporating. Another thing I want to use from your code at some point is the floating action menu. I looked it up and I'm really impressed by it. The one thing I would comment on is your shop activity xml. I didn't get to see it on a device but I see a lot of views on this activity. It may get too cluttered but I'm sure it will look great when I see your presentation. For me, as a personal preference, too much information can be overwhelming. Otherwise looks fantastic.
Is your peer making use of proper UX patterns we learned in class? If not, what are they doing that is unconvetional or that might confuse a user ( you )?
Is your peer making anything cool or awesome that you would like to note or applaud them on?
3. Does the project appear to follow correct coding styles and best practices? Write up one sentence on your findings and give a score 0-3.
Score 3: Code makes a lot of sense. Clear and concise. Not a lot of repeated code. Directories are clearly broken up and I love the conciseness of each method. Probably at the end, add some comments? Really my only addition to your code.
Are you able to reasonably follow the code without having anyone answer your questions?
Are you able to make sense of what the code is doing or is trying to do?
4. Find two pieces of code of any size: one that is readable and easy to follow and one that is difficult to follow and understand.
ShopActivity: In the initFirebase method I really liked how you broke up each of the sections for your database. This I'm sure will make it much easier to handle the data and be able to bring down reviews and ratings later in the app. It is better than the generic "push" method which will create keys that are just strands of numbers, symbols, and letters.
It is really difficult for me to find any sort of code that wasn't readable or hard to understand. I didn't see all of your models being used. I assume I either missed it or it was because you figured out that yelp has all the models built for you. The last really nitpicking thing I can think of is you have a boba fragment, and a tea objects in your code. As a boba tea novice, I'm sure those terms are interchangeable but it can be confusing to have both in your code.
What makes the readable code readable? Be as detailed as you can in your answer, it can be challenging to explain why something is easy to undertand
What makes the difficult code harder to follow? Be as detailed as you can in your answer.
5. High level project overview: Take a look at as many individual files as you have time for
_Remaining general comments:
I saw on one of your yelp calls you used location, while the others were coordinate options. I think it could be a cool feature if you gave the user the opportunity to search for a custom location as well as find what's close to the phone. An example of when this would be good is when someone is going to travel.
Looks great man. Wish I could give more criticism but I can't find any. _
Does this class make sense?
Does the structure of the class make sense?
Is it clear what this class is supposed to do?