Project 1 Feedback

[aleksandrTmk]

RUBRIC

Project 1 | 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)
READ.me is created				x
View a collection of to-do lists			x	n/a
View items on a to-do list			x	n/a
Allow the user to create a new to-do list			x	n/a
Add items to each to-do list			x	n/a
Display correctly in both landscape and portrait orientations		x		n/a
Bonus: Show an error message if invalid input is given	n/a	n/a	n/a	x
Bonus: Allow the user to check off and remove completed items	n/a	x	n/a	n/a
Bonus: Add an item detail screen that allows the user to give an optional description for each item	n/a	n/a	n/a	n/a
Bonus: Delete master list items	n/a	n/a	n/a	x
Bonus: Delete sub list items	n/a	n/a	n/a	x

Notes:
Beautiful code, I'm glad you went back and made it look this good! Great job with completing the bonuses and going above and beyond the requirements! Keep it up.
Note that part of the requirement was to handle orientation changes which you app does for the sub list screen but not the main task list screen.

---

Score:

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

Your total score is: 12

---

[aleksandrTmk]

@cherence The code is very very easy to read. You did a great job refactoring it and making it beautiful! Its good enough to make me want to show the rest of the class as a model, with your permission.

• Easy to read code
• Natural flow in terms of moving from one thing to the next
• Method names clearly explain what each method will be doing
• Excellent use of constants and not hard-coding strings into the app ( i.e. DATA_INDEX_KEY, etc ).
• Great job handling user input in all cases
• Nice touch on making the UI, clean, custom and "yours", it is your app after all!
• Very thorough READ.me file.

Below is some feedback to make you an even better developer overall:

Orientation Change requirement:
Your application does not handle orientation state changes in MainActivity.java. You need to useonSaveInstanceState() and onRestoreInstanceState(), take a look at http://developer.android.com/guide/topics/resources/runtime-changes.html

MainActivity review:

• Excellent coding style. This really is a beautiful class to read.
• There are no extra new lines or hardcoded variables.
• All methods have a purpose and do ONE thing and do it well.
• To improve yourself further, after line 71 you could do a null pointer check to make sure the program won't crash, like so: if (userInputLists == null){ return; }. This can be applied to all of your methods because you never know when listListView might be null, it might even be the case just as you are about to set you listListView.setOnItemClickListener().
• For listListView, the variable name itself is a bit confusing/redundant. listView is perfectly appropriate for this case.
• line 105 uses a hardcoded string value that should be turned into a constant ( which will then be used in DetailActivity.java
• onActivityResult() looks good at first glance, but remember to make sure your starting and closing { / } are in the correct positions. Lines 121-125 all need to be tabbed back.
• Please add some comments to your code! This will help other develops and yourself when you have long forgotten what this project is about. A good example where to put this is around the logic inside your onClickListeners() because those have complex logic, whereas your method names are so clear - a comment is not required.

DetailActivity review:

• Again, great job with refactoring! Code is clean, easy to read, concise and easy on the eyes!
• Line 59 uses a hardcoded string value ( same one as MainActivity.java line 105-106. If this was a public static final String TITLE_STRING = "newTitleString";, then here we could change String extra = getIntent().getStringExtra("newTitleString"); to String extra = getIntent().getStringExtra(MainActivity.TITLE_STRING); which is the proper way to do it.
• Lines 122, 130, 137 have a weird convention for naming intents as intent1, intent2, intent3. Leave each of these as Intent intent2 = getIntent();. Remember they are local scope to each method and are not visible to other methods. This is why you can reuse the same names.

Bug: Strike Through

• There is a bug I found with the strike through feature. If you add items item 1, item 2, item 3 to the TO DO LIST, and strike through item 2, item 3 will move up to item 2 position and will actually have a strike through!
• Lines 108-116 you have strike through logic which holds ONE member variable for ALL list items. This means that if I strike through item 1 and try to strike through item 3, it will fail because the strike boolean is true and will not strike through. The solution is to keep track of strike through for every item in the list which would involve keeping track of the states in another list and syncing up the indexes. This is where a class for Item.java would be useful and would have a member variable private boolean strikeThrough = false;

Good job with the project. Keep the code clean, you are doing it quite well now! Add some comments for more complex parts of the app, its good practice.