Skip to content

Project 4 Feedback#3

Open
aleksandrTmk wants to merge 1 commit intoblakepolidore:masterfrom
ADI-SF-Student-Projects:master
Open

Project 4 Feedback#3
aleksandrTmk wants to merge 1 commit intoblakepolidore:masterfrom
ADI-SF-Student-Projects:master

Conversation

@aleksandrTmk
Copy link

RUBRIC

Project 4 | ADI

Performance Evaluation

Mark boxes with an 'X'; if did not achieve bonus, mark with a '-'

Requirements Incomplete (0) Does Not Meet Expectations (1) Meets Expectations (2) Exceeds Expectations (3)
* Project Workflow: Did you complete the user stories, wireframes, task tracking, and/or ERDs, as specified in the requirements? x
* Technical Requirements: Did you make proper use of git, branches, etc? x
* Technical Requirements: Build a complete application x
* Technical Requirements: Have an impressive design and user experience that follows Google's Design Guidelines and can impress future clients and employers x
* Technical Requirements: Use at least one API or SDK x
* Technical Requirements: Be object oriented x
* Technical Requirements: Be robust, and handle cases of failure well (e.g., failed network calls) x
* Creativity: Did you added a personal spin or creative element into your project submission? Did you deliver something of value to the end user (not just a login button and an index page)? x
* Code Quality: Did you follow code style guidance and best practices covered in class, such as spacing, modularity, and semantic naming? Did you comment your code as your instructors as we have in class? x

Score:

Based on the requirements, you can earn a maximum of 18 points on this project.

Your total score is: 18

@blakepolidore Great job, solid looking project! UI looks great, code quality is awesome!

@aleksandrTmk
Copy link
Author

aleksandrTmk commented May 12, 2016

@blakepolidore Overall this is an excellent project.

Code quality is awesome, you have great structure. Lots of packages that make sense. I would make a top level models package and move foursquareModel and rottenTomatoesModel under that package since these two relate but are separate.

Lower score related to gitHub is that I saw no evidence of branching. Usually I'd see at least a few commits stating "Merging branch X into branch Y" in the commit logs. You did a great job with commits themselves and I can definitely follow along and see the progress. The feature branches just make your life easier and reflect a real word work environment. Even during solo development is pays off to use branches so I'd encourage you to try. If you did branch and the commits don't reflect that, let me know we can update the score.

On a higher level, I ran your application on my GenyMotion emulator instead of a real phone. Just to change things up. Ran into a few bugs and UX issues.

  • The drawer will not close unless I press the back button, which crashes your activity.
  • The crash has more to do with the fact that my emulator is missing google play services. This means that location, etc does not work. Your app does not handle this limitation nicely.
  • To prevent crash you could and should use
final int resultCode = GooglePlayServicesUtil.isGooglePlayServicesAvailable(activity());

This way if the play services is not available, then you can prevent crashes and either ask user to install it or change up their in app experience. This is not a bug I expected you to find/test but the point here is that its important to test on more then one device. Tablets from class, your phone, emulators, etc. Since Android has lots of fragmentation and the "it works on my phone" argument does not roll well in a work environment when you're expected to support many devices.

This is the reason for lower score in the handling errors, etc category of project. Its tough to test it all but as developers we need to make time to test the things we build and not rely heavily on QA or users to do it for us as they are bigger stakeholders then us.

Project feedback:
Some of the class names like Project4 for your application class isn't descriptive, and doesn't tell me much about what it is or would do. Main3Activity is also a name. I know you are in the process of cleaning things up though so not necessarily worried about it from your part.

Main3Activity.java is very long, try to think of ways to make it shorter. You can move some of the Api callbacks into classes, along with click listeners, etc. Or perhaps a manager class to manage state of things and move them out of Main3Activity. This way, this manager could be instantiated in another activity and re-used.

CardModelAndAdapter package is doing more then one thing. Just use an adapter package and hold the adapter there. CardModel could go under models package I noted above but maybe not within any subpackage.

Same for your forsquare adapters. It would go into the above Adapter package, but your click listeners and interfaces should go into their own packages that make sense. Same for swipeFling package.

Idea with packages is to group related things such as all adapters in one place, all listeners or helper classes, etc. This makes navigating the project easier, adding new types of these objects is simple and you know where to put them. It ups the organization to a whole new level.

You do not have any Unit Tests or Instrumentation Tests. I highly encourage you to write some, they look great for employers but more importantly they test your code. And they force you think about the classes you are designing because its one thing to make a class, its another to make one that is testable!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant