HackerNews Android - Get a Job
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 Job
s 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 if
s 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.