Android HackerNews - Trying to get to a Story
I'm not sure where this post is going to go.
I'm checking my pre-planned posts and ... there's none for the Android HackerNews Reader. This is kinda sad as I was enjoying being able to just sit down and start up on the project.
Now I've gotta THINK... bleh... terrible.
Anyway - Thinking about what I've done... and of course LOOKING AT THE CODE - I see that I've still gotta do the Story activity.
There's two ways I can do this; one - Swipe and go; or tap and go.
We have the tap already wired up; but I can tell I'm avoiding swiping because I've never done it before. Therefore; I must swipe.
I try to pay attention if I'm avoiding working on or implementing a specific something. If I am, it's usually because I don't know how to do it. At least not in detail. Given that I really don't like not knowing things; need to dive into that which I'm avoiding.
In this case; how to swipe.
Fundamentally; no idea. Time to hit the Googles.
After a little bouncing around I think I'd work with AndroidSwipeLayout if I was doing an app. It gives the flexibility to do a lot.
Currently; hiding all the functionality is going to be troublesome; especially since I'm going to be trying to TDD this... new post idea - "Testing with libraries" UI and Code.
Ignoring the powerful tools to understand the touch system better; I found
http://stackoverflow.com/questions/30908068/android-listview-swipe-right-and-left-to-accept-and-reject which leads to some decent baseline for implementing a swipe. It looks like I'm going for an onTouchListener. Cool.
Hopefully my UX friend will drive me to do more advanced stuff; forcing me to re-evaluate what I do... until she awe's me with her UX design; I'll do it like a dev... shudder I blame you Sam!
The Testing
It looks like I'll be writing tests for the TopStoriesAdapter... which is where the MASSIVE tests live... these need to be cleaned up now.
I'll be doing another one of these and it'll be C&P of ginormous. It's getting ugly. Let's focus on fixing that before we implement this new feature.
I guess instead of new tests; we'll be refactoring our existing tests into something not terrible. I'm not calling it ruthless refactoring because... well.. it's been there a while and there are three tests that have most of the code duplicated.
Because it's a huge blob of code; I'm not going to reproduce the tests initially. I hope to produce snippets as I go through. For an understanding of what I'm starting with; please see TopItemsAdapterTests as it exists at the start of this post.
....
And I made a bunch of changes and multiple tests broke. It was about 20 minutes of work. All simple things. No way it'd break... right... I expect that'll never stop biting me.
After a git reset --hard we'll start again. This time... we'll run our tests first. I suspect one of them is failing. Which I may have called out in my last post... Not checking; just rings a bell.
Yep - It's broken
java.lang.RuntimeException: Method setText in android.widget.Toast not mocked. See http://g.co/androidstudio/not-mocked for details.
So... Time to get some actual tests in place for that... Which I need to do before massive refactoring of these tests. OK - FIND THE GREEN!!!
I assume the issue here is that I'm not setting the Toaster mock Toast. Yep. I had it in the Job test, not the Story. As soon as I added
Toast mockToast = Mockito.mock(Toast.class);
Toaster.setReplacementToast(mockToast);
The test passed!
Now I'm green. I've got a bit of a hack and unorganization in the onBindViewHolderShouldSetStory which is the one that was failing. I have some "Arrange" code (see above) in the "Assert" section. If this wasn't one of the big ones that'll get refactored, I'd clean it up first. With the massive refactor of these tests next up; "fixing" this right now is a waste of time.
Refactor!
OK, for reallies now - Refactoring.
I did some "Extract Method" and... a test started failing. This makes minimal sense... so... Reverting and going to baby step it.
One would think that since I did this before, saw it broke a test... I'd kinda expect it to fail and take more precautions... but Noooooo... I run headlong into it again.
All tests pass after the revert. Good starting point.
Step 1: Extract the LinearLayout mocking to a method.
Run Tests: Pass
This step turns the following
{
final LinearLayout container = Mockito.mock(LinearLayout.class);
Mockito.doNothing().when(container).setOnClickListener(containerCaptor.capture());
Mockito.when(mockView.findViewById(R.id.ll_item_container)).thenReturn(container);
}
into
{
final LinearLayout container = configureViewLinearLayout(containerCaptor);
Mockito.when(mockView.findViewById(R.id.ll_item_container)).thenReturn(container);
}
Step 2: Extract a common QacTextView method for mocking.
Run Tests: Pass
This causes similar results for the QacTextView items.
Step 3-6: Same as Step 2
Run Tests: All Pass
Step 4: Inline LinearLayout Variable
Run Tests: Fail!
This part changes
{
final LinearLayout container = configureViewLinearLayout(containerCaptor);
Mockito.when(mockView.findViewById(R.id.ll_item_container)).thenReturn(container);
}
into
Mockito.when(mockView.findViewById(R.id.ll_item_container)).thenReturn(configureViewLinearLayout(containerCaptor));
The error from the test is:
org.mockito.exceptions.misusing.UnfinishedStubbingException:
Unfinished stubbing detected here:
-> at com.quantityandconversion.hackernews.screens.topitems.TopItemsAdapterTests.onBindViewHolderShouldSetStory(TopItemsAdapterTests.java:172)
E.g. thenReturn() may be missing.
I'm inclined to call this a bug in... something? Mockito?
Seriously; I'm transforming
final LinearLayout container = configureViewLinearLayout(containerCaptor);
Mockito.when(mockView.findViewById(R.id.ll_item_container)).thenReturn(container);
into
Mockito.when(mockView.findViewById(R.id.ll_item_container)).thenReturn(configureViewLinearLayout(containerCaptor));
First version, tests pass. Second; failure.
Ahhh - If I'd read the ENTIRE error message... I may have seen
you are stubbing the behaviour of another mock inside before 'thenReturn' instruction if completed
Which... Well... Yep. Apparently that's not an option with Mockito. Explains the previous failures.
Which means we need to leave it in the two line form
final LinearLayout container = configureViewLinearLayout(containerCaptor);
Mockito.when(mockView.findViewById(R.id.ll_item_container)).thenReturn(container);
I don't like this; so I'm extracting these lines into their own method.
Remember; we're trying to clean up the giant cluster that is this test. It's huge; this shrinks.
Doing the method extraction for each QacTextView simplifies the test code and all tests continue to pass.
As I continue to refine the simplification of the mega-test I'm shrinking call for each control down.
... Except I went down a path without running the tests constantly.
I don't expect straight tool based refactoring to break things... BUT... ... undoing things until the tests pass again. it's just one failure; I could probably hunt it down. Not going to. Rolling back until passing; then will move forward again.
Huh... moved the declaration of a field to the top... BROKEN.
There might be some oddity with Mockito and ordering. shrug I'll leave it for now. Go back to the transformation bit.
At the moment my "control code" looks like
final ArgumentCaptor<View.OnClickListener> containerCaptor = ArgumentCaptor.forClass(View.OnClickListener.class);
configureItemContainer(containerCaptor, mockView);
I'd like to get this down to one line. Which is what I had before the rollback.
The big aspect has been that the captor is required for "Assert"ing. Which means we should be able to push creation into the configureItemContainer and return it.
Let's go for that.
Now I've got the configuration of the LinearLayout down to
final ArgumentCaptor<View.OnClickListener> containerCaptor = configureItemContainer(mockView);
I'm going to go through a similar process for the rest of the controls.
There is an additional refactor I see coming; but will wait until I get a few more of the controls migrated.
It didn't take long to get a couple more migrated. What I expected has shown itself. Two of the QacTextView configurations look like
private ArgumentCaptor<CharSequence> configurePostedTime(final View mockView) {
final ArgumentCaptor<CharSequence> captor = ArgumentCaptor.forClass(CharSequence.class);
final QacTextView qacTextView = configureViewQacTextView(captor);
Mockito.when(mockView.findViewById(R.id.tv_posted_time)).thenReturn(qacTextView);
return captor;
}
private ArgumentCaptor<CharSequence> configureComments(final View mockView) {
final ArgumentCaptor<CharSequence> captor = ArgumentCaptor.forClass(CharSequence.class);
final QacTextView qacTextView = configureViewQacTextView(captor);
Mockito.when(mockView.findViewById(R.id.tv_comments)).thenReturn(qacTextView);
return captor;
}
Might be a little hard to read on the screen; but can you spot the difference?
There's just one. The R.id.* value.
If we extract the rest of this into a common method; massive code duplication reduction.
I want to stare at this a bit more before I go refactoring; see if I can dance things around a little and clean it up more.
A smidge cleaner as I was hoping. The generation of the Captor is now
final ArgumentCaptor<CharSequence> postedTimeCaptor = configureViewQacTextView(mockView, R.id.tv_posted_time);
a nice simple single line. The configureViewQacTextView is
private ArgumentCaptor<CharSequence> configureViewQacTextView(final View mockView, final int resId) {
final ArgumentCaptor<CharSequence> captor = ArgumentCaptor.forClass(CharSequence.class);
final QacTextView qacTextView = Mockito.mock(QacTextView.class);
Mockito.doNothing().when(qacTextView).setText(captor.capture());
Mockito.when(mockView.findViewById(resId)).thenReturn(qacTextView);
return captor;
}
This is a little longer than I'd like. But aside from pulling out a couple lines for building up the qacTextView not a lot I see to shrink down. I removed the method that did this. Not sure which way I prefer. Leaving it bigger for now.
I have two methods, one for the TextView and one for LinearLayout which come to something very similar
private ArgumentCaptor<CharSequence> configureViewQacTextView(final View mockView, final int resId) {
final ArgumentCaptor<CharSequence> captor = ArgumentCaptor.forClass(CharSequence.class);
final QacTextView qacTextView = Mockito.mock(QacTextView.class);
Mockito.doNothing().when(qacTextView).setText(captor.capture());
Mockito.when(mockView.findViewById(resId)).thenReturn(qacTextView);
return captor;
}
private ArgumentCaptor<View.OnClickListener> configureItemContainer(final View mockView) {
final ArgumentCaptor<View.OnClickListener> captor = ArgumentCaptor.forClass(View.OnClickListener.class);
final LinearLayout container = Mockito.mock(LinearLayout.class);
Mockito.doNothing().when(container).setOnClickListener(captor.capture());
Mockito.when(mockView.findViewById(R.id.ll_item_container)).thenReturn(container);
return captor;
}
These are super oddly alike. Which, after some playing, if it'll be made into one; it'll be so convoluted it won't make sense or be easy to use.
4 Rules of Simple Design
- Tests pass
- Elequently Named
- No Duplication
- Fewest Elements
These are in order of importance. Another way to look at #2 is that it convenys intent. If I refactor these two methods into one; and it's a crazy world of generics and lambda's... that's not going to convey intent. It won't be well understood; hard to understand code is hard to use.
I feel it'd violate #2; so we keep the duplicaion.
The code is easier to understand which makes it easier to maintain.
When I run the tests; I'm thinking, "I really hope they all pass". Not because I would then have to hunt down a bug... but because I just undo until I get back to passing tests. I've given up hunting down bugs when I have TDD. I just try again. It's faster in the end.
Looking at the test some more. I see that the generation of test data and the validation of the test data takes up 15ish lines. I have a StoryBuilder class as a helper... I wonder if I can leverage that to reduce the code in the test itself...
Mockito.when(mockTopItemsActivityMediator.itemAt(position)).thenReturn(
new StoryBuilder()
.setTitle(titleExpected)
.setAuthor(authorExpected)
.setCommentCount(commentsExpected)
.setScore(Integer.parseInt(scoreExpected))
.setPostTime(Integer.parseInt(postedTimeExpected))
.buildStory());
What if instead of creating the values; storing the values, then using the values; they were created in a method and stored as part of the StoryBuilder?
I did this refactor (it's a bit of code to paste in) and it removes about 9 LoC; but it removes 5 variables.
I've gotten a bit far with the refactor of the test.
It's now 32 LoC; which; Ewwww - I agree. I just don't have a better way right now. Maybe it's because I'm doing my Adapter wrong; I'm OK if that's the case. I just don't know how to improve it.
For now; I've reduced the test method from 83 lines, to 32; a savings of 50 LoC
Even adding in all the helper methods; the total LoC is ~73; saved 10 lines.
Across the 3 methods; really 4, because I need this stuff to get in the swipe mechanism... Right? I almost forgot the point as well.
Given the 3 existing methods; 150 LoC saved. The methods are FAR FAR FAR easier to understand.
If you want to be really picky; there's 20 lines in the StoryBuilder as well. :-P
Over 100 LoC removed. And that will only be an increasing savings.
Further De-duplication
I wonder if I'll be able to have a base method for these tests. I expect most of the code will be the same; it was before. Minus a few variations.
Guess I'll have to update the other tests and see what shakes out for uniqueness.
I got one other test updated to the new style; there are 3 lines different.
There's not a super clean way I'm aware of to put things into a common method.
I'll have to look at this some more when I have additional sit down time.
This sit down will probably involve a half dozen refactoring and pattern books to see if somethings stands out as a solution.
It feels like I'm gonna have a helper class that encapsulates the captors; and then it gets called to do the validation.
We'll see when I get back to this post...
time passes
OK; Sitting at the cheer studio again - I've refactored the commonality into a single class. There's a bit more I could force an abstraction out of.
shrug
I'm not worried about pushing it to the limit. It's functional for now.
Also; the previously 80+ LoC method is now <10 lines.
This is a win. I had 250+ LoC for three methods. These methods are now <30 LoC.
Everything to do this takes ~115 lines. This is still only 150 LoC. If I add a new test... which... ya know... WAS the idea when I started this post - It'll be ~10 LoC; not another 80.
My support class will evolve and grow; but it makes the test clean and easy.
This is a far longer post than "refactoring tests" should be; but it's an awesome result.