The Top Stories apparently can include job postings. I have the StoryAdapter only accepting responses that have the type of "story". We need to build up a path for Jobs because it's in the payload. Well... mostly because it crashes the app when I scroll.
Network
We can assume that we'll have a Job class like we have a Story class.
Gonna swing in the test class to TDD it up. looks around Still not paired.
Job is going to be VERY similar to Story; I'll just C&P the tests and classes around - I know; Boo. But it's quicker. C&P means an abstraction is missing.
I expect that I'll end up back at "item" types. At least as a base class.
The first class I'm C&Ping over is StoryId. This really should be abstracted.
Then... StoryId and JobId will be empty classes... and we don't need that. Which means that ItemId will have a createStoryId and createJobId which will use slugs as identifiers... yea... I guess so...
Then add a isStoryId and isJobId... Yea...
This has turned into less of a C&P and more of a C&P means abstraction; so abstraction.
OK; ONTO THE CHANGES!
I did a rename of StoryId to ItemId. Added a STORY_ID and JOB_ID slug; and the instance var to hold it.
Privatized the constructor and replaced all constructions with createStoryId which provides the slug to the private constructor.
The NULL_ITEM_ID has a null slug.
Some string updates and all 119 tests pass!
We need to rename the StoryIdTests to ItemIdTests and TDD up some checks for the isStoryId method.
Which now works wonderfully. I used the NULL_ITEM_ID to validate that isStoryId returns false.
We now can create isJobId. This will help drive the rest of the Job related classes.
Looking at the Story class and the hacker news api doc; only the ID is a common field; I don't know if that's enough to create an abstraction. MAYBE? If it shares enough functionality, it would make sense. I'm not going to force it right now; just slowly build out the Job related classes like I did for the Story.
The doc's have ID as the only REQUIRED... all items appear to have more commonality.
"by" : "dhouston",
"id" : 8863,
"time" : 1175714200,
"type" : "story|job|..."
This is enough that an abstraction is warranted.
Item pulled out; Story now extends Item.
I'm kinda waffling on if score should be in Item. It makes sense right now as it's shared between Story and Job... and they'll have duplicate code...
Red - Green - Refactor.
I'm trying to pre-optimize. That's a no-no. I'm going to leave Job in; and empty!!! There will be a "URL" field. Which Story will also have, so the thought still applies. Right now I'm trying to avoid a crash when a job comes in, let's get that working.
There's been a significant chunk of refactoring around creating the required Job classes. Now to test it!
... Well Crap... ish. There's a Job coming back in the top 20 or so... which is crashing the app... :\ hehe
This isn't working
SIGH So; there needs to be a much heavier refactor of Story to Item. I can probably hack some stuff in; but.... Noooo.... I'll have to set aside a few hours to heavily refactor so that the StoryAdapter is an ItemAdapter. This is pretty much required for what I want to accomplish. ... yay.
Until I get the time to hammer at that - ... I would ask you to wait... but this is a blog; you'll just continue reading the result when I type it out. Lucky you!
I really enjoy the irony that I started with Item classes and changed to Story and am now having to go back to Item. Which I fully support because we didn't know we'd need it.
Continuing the trend of not distinguishing between Story and Job data - The HackerNewsNetwork class with the separate story and job method... Nope. Need a single one.
There's a big dance of refactoring going on here... It's kinda crufty to see what needs to be done; but I expect beauty when it's complete.
I wrote a test to test the TopStoryAdapter#onBindViewHolder to bind a job. Which will need some work as there is slight variation between a story and a job ui.
This test is what's driving a lot of other testing. I've commented out the driving test; and am ensuring I fix each failing test as I make changes.
This is a time when running tests like crazy is needed. The smallest step; make sure it works.
AHHHHH! 7 FAILING TESTS!!! UNDO UNDO UNDO UNDO UNDO UNDO UNDO
I changed two things. Bad me.
I was struggling along for a while; hunting down the code I missed in the story->item migration. I had 5 failing tests.
Finally traced it down to a section checking specifically for story id's. Not sure how it was missed originally - And BAM! all tests work.
Now to go re-enable the original driver test. And run it to make sure it still fails. Yep; fails. Imagine that - It doesn't display the job correctly... since I haven't implemented it!
One thing that the code keeps YELLING at me about is that Job and Story should be encapsulated in Item. I keep getting directed to add a isStory and isJob to Item. Base classes should not know about their derived types; so this doesn't work out well.
I think I've gotten past the little hiccup I was having about migrating it in. I was trying to take a big bite; now I'm doing a lot of small stuff.
I put in create methods that just called the Story and Job constructors.
Then built up all the checks around while having it go Item->Job->Item.
I really hope that this process of tearing out the Story class doesn't backfire into painful test failures... And there's just 1 failure! Which is a string that got overly refactored in the test project.
I now have no Story or Job class. All functionality still exists... Well; tests still pass.
A huge boon of having the Adapter covered in tests; I /know/ that it will work for either jobs or stories. I have tests that show it.
There's a little bit of renaming to do to eliminate the reference to Stories; but, small detail.
I now need to run the app in an emulator to just confirm that it's working right.
I should really write a UI test to check this for me... But it's 1am; so... It gets shoved to the back.
All told; the refactor once I sat down took about 3, maybe 4 hours. What I expected. My test count ... went up 5. Neat; and class count went down by 2.
Running this on the emulator... apparently some files aren't closing...
E/NativeCrypto: AppData::create pipe(2) failed: Too many open files
Ahh - Yes. Network. I'm spinning up a new OkHttpClient for each request. I'll have to optimize how the OKHttp client is handled. I suspected I would early on; but now I know; and now is the correct time to do it.
less optimize; more... not be broken.
Doing it "now" being, later.
Remarks
I want to highlight that I would have spent WAY more than 4 hours trying to remove the Story and Job classes. To get them into the Item class. I probably would have never done it. Just hacked in ifs to see what type it is.
The work I'm doing here exemplifies why the XP practices make coding better. I am reducing the class count; increasing the test coverage and simplifying the code just by applying good object oriented designs.
It shows that having solid test coverage allows focus to remain on feature work and not trying to verify functionality.
Did you pick up on the lack of modification to the UI to support displaying Jobs?
I could have just left out the changes, sure; I didn't. There were no changes to support displaying Jobs. I expected there to be. My driving test was around the adapter to see jobs displayed. Once everything was an item. BAM! worked. Using a lot of Object classes and the Null Object pattern allowed customization to the outputs in those; instead of hacking them into, and requiring, Story and Job classes.
The result; It. Displays. It came out far cleaner than I expected. I'm thrilled the practices are showing their immense value.