Project 3 Feedback

[aleksandrTmk]

RUBRIC

Project 3: Vice | 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)
Include wire frames			x	n/a
Include user stories based on your research and feature prioritization			x	n/a
Hit the Vice API			x	n/a
Allow the user to browse through Vice's news articles using an interface that is photo-based			x	n/a
Allow the user to click on a photo and read the article to which the photo relates			x	n/a
Provide the user with some mechanism by which to filter stories based on one or more of the following: Keywords, Tags, Topics, Location			x	n/a
Integrate with the Twitter and/or Facebook's APIs			x	n/a
Allow the user to share individual photos and stories via social media			x	n/a
Look great in both landscape and portrait modes and reflect Material Design principles				x
Not crash or hang and should handle for when networking/internet is slow or unavailable			x	n/a
Include at least one Notifications feature (e.g. reminder, alarm)			x	n/a

Score:

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

Your total score is: 23

Great job with the project team, the app is awesome!

[aleksandrTmk]

@adao1123 @salminnella @StarAceM @stewartmcm Great job with project 3! You guys did an awesome job improving upon what the Vice app already does, along with adding great new features!

Impressed by the fact that you really considered the UI/UX and created separate layouts for mobile, tablets, and horizontal/portrait tablet orientations at that!

Excellent use of packages to organize your work!
Glad you learned about some new libraries to make your lives easier,
never re-invent the wheel unless is absolutely necessary and you can
make a better one.

Note you are missing unit tests. The wire frames could and should be more detailed. Feels too barebones right now.

Great job with trello board for organization!
Nice work with github, splitting up the work, using branches and committing often!

Here are tips on improving yourselves as developers in the future:

MainActivity.java review:

• Great job with regions and variable names to keep everything readable.
• Nice job with the comments and keeping methods clear, with a single task to accomplish.
• onCreate() is getting long. An idea to shorten it would be to move toolbar setup to its own method. Then moving all view inits like initViewPager(); initTabLayout(); into an initViews() method, initHelpers() for fb, etc.
• Clean up extra white lines and remove commented code in onCreate().
• getPrefsForDrawer() is a nice small method. Note that the "get" name part implies we are going to set a return value, while the return type is void. This means the other two methods inside, createBoolArrayList() and setNavigationDrawer() would have to change how they "work" in terms of what is returned or passed as an argument. All this will make the code more readable.
• unbindDrawables() should be called in onStop(). The reason is that all clean-up should be done in onStop() and not onDestroy(), as you've learned in the mock interview. Remember this going forward.
• setNotificationAlarmManager() still uses hard coded key value strings for intent extras. If you wanted to re-use these or change them, you'd need to hard code everywhere else. This is why making them constants is more helpful.
• Long alertTime = new GregorianCalendar().getTimeInMillis()+7*1000; Long intervalTime = 120*1000L; I would also make these ints constants for easier re-use and updates later!
• isNetworkConnected() is used in MainActivity.java and LatestNewsFragment.java, using the same code in two places. Remember the D.R.Y. principles and write code only once and re-use it! Moving this to a Utility class that both Main and Fragment then access statically is a better choice. Reason being is that if you ever wanted to change how you check the network, i.e. only care about wifi or cell signal then you'd update code in one places instead of 2+.

ArticleActivity.java review:

• Overall great looking class. Nice work with comments, keeping methods clean and variable names clear.
• onCreate() is short and looks great. The only thing i'd improve is removing those in-line comments. They aren't necessary since method names tell me what each will do, and each method has java docs explaining further.
• onOptionsItemSelected() could be shortened by creating helper methods, ie handleBookMarkItem() and handleShareItem() for each if-else statement.
• onDestroy(), all the work here should really be done in onPause() - it is considered part of the data saving process. Refer to mock interviews for guidance on this.

ArticleAdapter.java review:

• Overall nice class! Glad to see you're getting comfortable with RecyclerViews.
• Great job using interfaces to communicate info back to your fragment!
• The class needs more top level comments.
• onBindViewHolder() does a good job making sure holder.previewTextView on line 57-58 is not null. Note that you already have titleTextView so no reason not to use that in line 58. imageView on line 62 is never null checked, you should do that.
• public static class ViewHolder there is no reason for this ViewHolder to be public. Also, try and make a more descriptive class name for what type of views this holder holds.
• line 87 nice work with using this trick for tablet layouts!

ViewPagerAdapter.java review:

• Nice work here, simple class does has one thing to do and that is to be an adapter for a view pager.
• mFragmentList and mFragmentTitleList are final, note that this means they cannot be re-assigned via new if you needed to. Which in this case, seems fine.

LatestNewsFragment.java review:

• Nice work with this fragment. Great comments, variable names and methods.
• Note: TOAST_NO_NETWORK should actually be defined in strings.xml since this is a user facing string that could be changed to another language.
• onViewCreated() is nice and short, great job. Its an easy read!
• Should makeRV() be called if there is no network connection? Is there anything to show since no API will return results?
• displayLatestArticles() is a nasty method and needs refactoring. There is a large number of indentations happening. For instance, you can remove the first layer if (!fragTitle.equals(getResources().getString(R.string.bookmarks))) { by making it check if title equals bookmarks, return. Then do processing below this. Same goes for if ( call != null ) can be turned into if (call == null) return;. Again, same for getActivity() null check. All the code from onResponse() should be moved to a helper method handleResponse(response); to remove the nesting even more.
• Note that DatabaseHelper searchHelper is instantiated even if it will never be used, ie when article count != 0. If you instantiate it inside the if statement, you save yourself one extra call since its only used inside the if's for-loop.
• isNetworkConnected() same comment as from MainActivity.java, move this method to a util class so its written once not twice.
• makeRV() needs more comments. It looks like you're making an animating adapter, but this should be explained in the java docs for the method. Otherwise if I used it, i'd be expecting to make a recyclerView but NOT expecting an animation along with it.

NavigationDrawerFragment.java review:

• Nice, clean class.
• Excellent use of comments, clear variable names and explicit methods that handle their job well.
• Nice use of interfaces to communicate data!
• onPause() clever solution for saving preferences.
• initDrawer() should just do that, and then setting the drawer on the recyclerView should happen in a separate method to keep things more clean.

RV_SpaceDecoration.java review:

• Glad you made the decoration a separate class so it can be re-used in other recyclerViews!
• This class should not live in fragment package. That package should strictly only hold fragments.

BookmarksHelper.java review:

• Nice work with this AsyncTask hack to get a looped number of results based on ids. Sometimes you have to come up with your own solutions when API's are limited.
• VICE_BASE_URL doesn't make sense to live here. Ideally it lives in ViceAPIService instead.
• doInBackground() the tasks inside should be split into methods to make doInBackground() shorter and easier to follow.

DatabaseHelper.java review:

• Great class overall. Nice use of comments and variables for constants.
• Method names make sense and handle their jobs accordingly.

NotificationDBHelper.java review:

• This helper uses the same DB: ArticleNotices and same table articles as DatabaseHelper.java. Its is, for all intents and purposes, a copy of the other helper that adds on some extra functionality.
• Why is this functionality not part of DatabaseHelper.java? There is no reason to have this class. You should just move these methods into DatabaseHelper.java since they are both acting on the same DB but are not following the D.R.Y. principles.

NotificationPublisher.java review:

• Nice idea with the broadcast receiver inside the alarm for notifications!
• This class name is deceiving. It extends broadcastReceiver, therefore it is a receiver class not a publisher. I do understand that this receiver then actually publishes the notification but a better name would be CreateNotificiationReceiver.javaor something along those lines. It receives a an intent to create a notification.
• Lines 25, 26, and 27 are using hardcoded strings when the first two keys should be using MainActivity.ARTICLE_ID_KEY and MainActivity.ARTICLE_TITLE_KEY. The last string for the notification message/title should be defined inside strings.xml so that it can be easier to translate later in the future ( this is a user facing string ).
• mNotificationManager.notify(1, mBuilder.build()); uses 1 as the notification id. This should be used as a constant as well. Think about it, if you want to remove this notification you will need to know its id, and if you use a constant you always know for sure what that id value is. Otherwise you have to guess and run the possibility of the hardcoded values getting out of sync.
  alertIntent.setAction()

Models review:

• ArticleArray.java and ArticleData.java are both the same model. Remove one and use only one, there is no reason to have two classes that model the same thing, it is only confusing.
• Rest of the models look great. Try to stay consistent with the @SerializedName, either use it everywhere or don't at all, it makes reading easier but is not a big thing here.

NavDrawer items review:

• Nice use of superclass property to be shared across all the entries via NavDrawerEntry.java. Maybe consider making a tag field or something so that every entry would have its own tag and its not an empty super class.
• NavDrawerItemWithIcon.java could just extend NavDrawerItem.java and add the private int iconId; plus getters and setters instead of writing the same class twice plus a new field.
• NavDrawerToggle.java should also extend NavDrawerItem.java since it needs a title and the item has a title field for use.
• NavigationDrawerAdapter.java looks great, nice job. The only thing would be to make sure that ItemWithIconVH class extends ItemVH because again, the latter has a title and the first one has a title and icon - and both would extend RecyclerView.ViewHolder since the latter extends it. ToggleVH should also extend ItemVH for re-using the title field.

NotificationIntentService.java review:

• Class needs top level comments explaining what it does.
• Not entirely clear if/when it is called. It is declared in manifest but not used anywhere.
• It seems it was supposed to be the NotificationPublisher.java class but that already exists.
• Structure wise, the methods need to be shortened. ie showArticleTitle() is too long and can be broken down with helper methods.
• Lots of strings here that need to be constants and or declared in the strings.xml. Line 65 and 53 in particular.

ViceAPIService.java review:

• Looks like a basic service. Does its job. Clean and neat, great job.
• All your get calls should be pre-pended with a "get" in their name, ie "getLatestArticles" or if it was a post call, would be postLatestArticles. This way one can know what type of call is being made without looking at the source code.

Authenticator.java review:

• Decent class but needs more java docs instead of the inline comments. Java docs look better to the eye and are easier to read. Especially when they are right above the method.

AuthenticatorService.java | SyncService.java review:

• Straightforward and clean. Does what it needs to.
• Needs more java docs and explanations.
• Not sure where it is being used, bounded to, etc.

StubProvider.java review:

• Great job, looks good.
• Missing java docs, the comments are great but remember that comments above a method should be made into java docs for readability and reference.

SyncAdapter.java review:

• Great job here, nice variable and method names.
• VICE_BASE_URL should live in the API definition not here.
• onPerformSync() the zero index could be made into a constant, ie FIRST_RESULT_INDEX or something along those lines. This way, changing one variable would affect all lines without manually changing them all.