VBM on Android - Top Stories Screen
OK - Looks like we've gotten to the point where building out a UI is gonna be a good next step. We need to know what we want to see to know what app innards to build up.
Sure, sure "Top Stories"; but honestly, I have no idea what's in a top story payload.
The topstories API is just ID's.
[13593814,13593272,...]
We can use the wonderful documentation and figure out what these ID's map to.
The documentation makes it pretty clear these IDs are "Items"
Stories, comments, jobs, Ask HNs and even polls are just items. They're identified by their ids, which are unique integers, and live under https://hacker-news.firebaseio.com/v0/item/.
Using the provided URL and the first item id; we'll checkout the json from https://hacker-news.firebaseio.com/v0/item/13593814.
{
"by" : "minaandrawos",
"descendants" : 127,
"id" : 13593814,
"kids" : [ 13595625, 13594550, 13595203, 13594237, 13594363, 13594373, 13595483, 13594264, 13594109, 13595452, 13595122, 13594229, 13595478, 13595366, 13594727, 13595318, 13594388, 13594221, 13594943, 13594892, 13594205, 13594073, 13594583, 13594339, 13594201, 13594400, 13594199, 13594519, 13595276, 13594394, 13594387, 13594585 ],
"score" : 319,
"time" : 1486508531,
"title" : "What programming languages are used most on weekends?",
"type" : "story",
"url" : "http://stackoverflow.blog/2017/02/What-Programming-Languages-Weekends/?cb=1"
}
Note: How long this ID and URL stay valid are not up to me
The documentation is pretty good explaining all these fields; and some not present; so go check it out for more info.
For the first iteration; just to get a UI in place and the new item API in place; I think I'll do a list of Stories that display the title, score and by fields.
Very simple; but gives a bit of variety... Or... to do the minimum first - Title. That's all. We're going to show a title.
Keeping it simple
There's a reason I want to keep it simple - Not to over complicate it as I drive functionality! Key Software as a Craft practice! Also; I'm going to use the Android RecyclerView; and it's a little different than I've done before - so I want to keep it simple initially. This is a UI component and I've not fitted it into the VBM process before. This is an opportunity I'll use to evaluate how additional UI components fit into the VBM (I suspect smoothly). Keeping the UI itself simple will delay the complexity for when I understand the structuring of the code better.
Recycler View
This requires a new dependency in the app's build.gradle.
compile 'com.android.support:recyclerview-v7:25.1.1'
Appium
Our UI is going to be updated; which will break the Appium tests. Actually - No. I'm going to create a new layout for the Top Stories. It'll require it's own tests. When I remove the MainActivity; then I'll remove the Appium tests. I don't want to have a broken tests hanging out.
This will give me a good follow up post utilizing Appium on more complex UI tests.
New Layout
Creating a new layout is pretty simple. I bet there's a hot key I don't know for it
Creating this prompted me if I wanted to use AppCompat or not - Which I do; and was reminded about creating a base 'activity' class. I have a few reasons for this; and it comes in handy a number of places. This consideration is to remove the knowledge of how activities are required to invoke the super class. For example;
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
}
If you don't call super.onCreate the app will crash. Sure; that'll ensure we call it; as well as the lint error.
I pref to not worry about these small details. As well as worrying about the order of when to initialize things; or create references.
I'll surmise that it'll feel like a bit of overkill the way I'm going to go; but let's see how it feels. If I don't like it - there's this amazing thing... REFACTOR! :)
This change will show up later as work on the TopStoriesActivity starts.
Layout
In our newly generated activity_top_stories.xml; which I wish was defaulted to top_stories_activity.xml for reasons that will become clear later - I am renaming it.
As I was saying; In our newly generated top_stories_activity.xml; we'll want to put in the RecyclerView
<?xml version="1.0" encoding="utf-8"?>
<android.support.constraint.ConstraintLayout xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:app="http://schemas.android.com/apk/res-auto"
xmlns:tools="http://schemas.android.com/tools"
android:layout_width="match_parent"
android:layout_height="match_parent"
tools:context="com.quantityandconversion.hackernews.screens.topstories.TopStoriesActivity">
<android.support.v7.widget.RecyclerView
android:id="@+id/rv_top_stories"
android:layout_width="match_parent"
android:layout_height="match_parent" />
</android.support.constraint.ConstraintLayout>
As we'll find through my naming; I like control type prefix (I don't agree that it's hungarian) and lower_snake_case for the IDs. If I was using Kotlin and Anko; then I'd go for camelCase due to the usage in code.
With the RecyclerView set in our ConstraintLayout we can move onto creating the item layout file named top_stories_item.xml.
You might be able to tell by this name why I like the ordered switched. It becomes much more obvious when more layout files come into play and when I start to create files in res/values.
Top Story Item Layout
One of the first recommendations the UI is making for the layout once I add a TextView for the title is to change the layout_width
I could just set it to 0dp; but that's a hard coded value that doesn't communicate intent.
What I'm doing, and this is part of my Android philosophy towards structuring files, is creating a values/common_dimens.xml file for common ... dimensions. Such as an entry for 0dp which reflects the intent.
<dimen name="layout_width_for_weighted">0dp</dimen>
And yes; while this may seem excessive; it is all by it's lonesome; but it builds up to create a powerfully communicative code base.
Unfortunately Android Studio is a bit too dumb and still pesters me about setting it to 0dp.
I'll add a tools:ignore to this control to not have the errant warning.
I create integer values like this for the weight as well. I find it reads cleaner when I can see what share of the space a control is going to take; a single_share or a double_share and on up.
The top_stories_item.xml now looks like
<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:tools="http://schemas.android.com/tools"
android:layout_width="match_parent"
android:layout_height="wrap_content">
<TextView
android:id="@+id/tv_title"
android:layout_width="@dimen/layout_width_for_weighted"
android:layout_height="wrap_content"
android:layout_weight="@integer/single_share"
tools:text="OMG - A TOP STORY!!! YAY!"
tools:ignore="InefficientWeight" />
</LinearLayout>
I want to point out here (because it cost me many hours) the android:layout_height is wrap_content. If you unknowingly (or don't realize it matters) leave it as match_parent the first item will take up the whole screen, and no additional items will display.
RecyclerView Adapter - TAG ALONG
Now that we have our layout; we can look at setting up our RecyclerView.Adapter.
I expect this to get a lot of refactoring so that I can find a clean way to Unit Test this and maybe not THIS one; but a future RecyclerView.Adapter can be TDD'd.
I'm not a UI guy; that'll end up showing in my amazing design skillz.
I'm working through example and guides to pull together something. The RecyclerView component here is pretty much a TagAlong section. I don't know what I'm really doing; so you get to see me bumble about.
To create and get to a compiling state; the TopStoriesAdapter looks as follows:
public class TopStoriesAdapter extends RecyclerView.Adapter<TopStoriesAdapter.ViewHolder> {
@Override
public ViewHolder onCreateViewHolder(ViewGroup parent, int viewType) {
return null;
}
@Override
public void onBindViewHolder(ViewHolder holder, int position) {
}
@Override
public int getItemCount() {
return 0;
}
public static class ViewHolder extends RecyclerView.ViewHolder{
public ViewHolder(View itemView) {
super(itemView);
}
}
}
The ViewHolder is what we use to perform the translation between the object and view controls. ... Which sounds A LOT like the B in VBM. I'm going to just get the Adapter working; kinda hacky; and then review how it can be refactored to be testable and possibly follow some UI design that's been discussed here...
The first thing to do is build out what little we need to in the ViewHolder to get a reference to the control
public static class ViewHolder extends RecyclerView.ViewHolder{
private TextView title;
public ViewHolder(final View itemView) {
super(itemView);
bindControls(itemView);
}
private void bindControls(final View itemView) {
title = (TextView)itemView.findViewById(R.id.tv_title);
}
}
As I go to implement the constructor of TopStoriesAdapter I find that I need to create a TopStories class... to serve as a collection of TopStories.
This should get interesting as an initial think through points out that we'll only have ID's from the topstory API. The TopStories is going to need to async collect data?... Hmmm... Interesting.
Which... Nevermind. This is where having a pair would be useful. I already have 'TopStories'; It's the 'Items' class.
Setting up the onCreateViewHolder was pretty straight forward;
@Override
public TopStoriesAdapter.ViewHolder onCreateViewHolder(ViewGroup parent, int viewType) {
Context context = parent.getContext();
LayoutInflater inflater = LayoutInflater.from(context);
View view = inflater.inflate(R.layout.top_stories_item, parent, false);
ViewHolder viewHolder = new ViewHolder(view);
return viewHolder;
}
Which is an overly long method. I fixed that...
@Override
public ViewHolder onCreateViewHolder(final ViewGroup parent, final int viewType) {
return viewHolder(parent);
}
@NonNull
private ViewHolder viewHolder(ViewGroup parent) {
return new ViewHolder(topStoryView(parent));
}
private View topStoryView(ViewGroup parent) {
return layoutInflater(parent).inflate(R.layout.top_stories_item, parent, false);
}
private LayoutInflater layoutInflater(ViewGroup parent) {
return LayoutInflater.from(parent.getContext());
}
While implementing onBindViewHolder it became obvious I'm missing a few things. I need to be able to get a specific item out of the Items class. And I need an Item class.
We'll go for the Item class first as that'll be easy to TDD.
Items
The first thing we're adding is an itemAt method. This will return the item object. Though... This does require an async operation...
I think I'll just return the itemId first; figure out how to pull data async; then push that into the Items.
Ahh... now I get to do my silly TDD of an interface... ... Nope. Don't wanna.
It feels dumb to have the tests creating the API; it works well pure TDD; but they are fragile tests with other paths ... no; it's an interface. There aren't other paths; there shouldn't be; It's. An. Interface.
Thank you me; we'll deal with that.
Renaming
Going through and adding the API call to get the individual story; I realized my naming convention was going to cause a lot of confusion.
While topStories API returns 'ItemId' values; if I have everything as an "item" I'll run into serious issues attempting to communicate clearly in code. There will only be items. For everything; it's all an item - That will become a giant cluster of confusion.
What I'm doing instead is creating distinct classes (likely to refactor commonality) and having a distinct API call for each. They will all point to the same endpoint; but the deserialization and resulting return type will vary. This will allow greater clarity in the code; which is only second in importance to tests passing.
If I never managed to refactor away the duplicity; it would be a better code base having the type distinction.
So I did that renaming.
Story Deserialization
One of the things I do with Retrofit and Moshi is have a "Json" class. This is where the data is deserialized into a package-private class. This class then is provided to the adapter and the return type class is built from the fields.
This allows the return type class to do validation at construction; no need to ... add hacks in... as I've seen done too many times. isValid ... bleh...
Anyway - The StoryJson class doesn't need testing. It's literally just fields.
I doubly like to use this pattern as it allows oddity in naming, without having to liter a primary class with annotations.
While I put everything in; I'm commenting out all but the title; the one piece of information we currently need.
...
A bit of coding later and the Story supporting classes are evolving out of the TDD.
The Story with just a title
public class Story {
private String title;
public Story(final String title) {
if(Strings.isNullOrEmpty(title)) { throw new IllegalArgumentException("title can not be null"); }
this.title = title;
}
@Override
public int hashCode() {
return title.hashCode();
}
@Override
public boolean equals(final Object other) {
return other instanceof Story && this.equals((Story)other);
}
private boolean equals(final Story other){
return this.title.equals(other.title);
}
}
The StoryAdapter using the StoryJson
/* package */ class StoryAdapter {
@FromJson
/* package */ Story storyFromJson(final StoryJson storyJson) {
if(storyJson == null) { throw new IllegalArgumentException("storyJson can not be null"); }
return new Story(storyJson.title);
}
@ToJson
/* package */ long storyToJson(final Story story) {
throw new UnsupportedOperationException("serializing to json not supported");
}
}
The updated HackerNewsApi with the story method
* package */ interface HackerNewsApi {
String URL = "https://hacker-news.firebaseio.com/v0/";
@GET("topstories.json")
Call<Stories> topStories();
@GET("item/{storyId}.json")
Call<Story> story(@Path("storyId") final int storyId);
}
Expose the Story
Now the HackerNewsApi#story method needs to be exposed via the HackerNewsNetwork and upwardly the HackerNewsAccess classes.
A lot like the main activity; these will be consumed by our TopStoriesMediator... when the time comes to create that.
For now; let's get the call exposed!
I'm starting with HackerNewsAccess. When I create the test for the story method; I realize I need to validate that the 'title' I expected is set.
I'm not doing getters. Getter/Setters are massive code smells. It turns the classes into a glorified databag and I'd like to have none of that.
Looking back on some experiences; I'd LIKE to have a few Story#title methods to write to different data types; StringBuilder, Ui Controls, etc. This is ideal; but Java makes it clumsy and ugly. C# has partial classes which make specific implementations far easier to stomach as the "core" class remains untouched. Java does not have this. These methods would cruft up the class.
The next option is to just implement the ones needed... but ehh... It'll feel sporadic and undesigned. Loose and hard to continue the practice/pattern.
I want to have a system of not doing a getter, but provides an example to use moving forward. ... With Java I think the answer is StringReader.
I'm sitting here arguing with myself over "Don't do a getter" and "Why do I have to go String->StringReader->String?!?!
I know - It's encapsulation of the data. It's preventing it from being a getter... but... WHY?
I'm missing a critical understanding on this - if I can't convince myself; what chance do I have for others!? This is actually a significant aspect to the VBM; I should grok it just a SMIDGE better.
Running ahead with StringReader for now. I'll discuss this with some fellow Software Craftspeople at work.
Anyway - Continuing the development; I've TDD'd the network layer for the Story. One change I had to make to invoke the request and keep data encapsulated is to move StoryId into the internal package. This allows exposing the id value as a raw type for consumption by the API w/o wider exposure. If StoryId ends up having to be moved out of internal then I'll adjust this to be ... some kinda Stream?
Now that I've gotten the Story networking aspects in place; I can work on the StoryAdapter; which is where I was before I had to go off into the weeds.
I'm looking at how to have the TopStoriesAdapter do the async call to get the hydrated story object; but not seeing a clean way yet. I'm putting in a TopStoriesMediator and I think it's the correct location; which then may remove the need for the TopStoriesAdapter to take the set. The Adapter may be part of the Bridge layer...
Gotta say; this is a bit of a random walk through testing. I'm bouncing around between TopStoriesActivityBridge, TopStoriesActivityMediator, and TopStoriesAdapter working on setting up the Story to be retrieved asynchronously.
One adjustment I made to simplify the ViewHolder is using the NullObject pattern for the Story. That way the ViewHolder just has to set values. No logic required. This does create a bit of complication in Story but it's a fine tradeoff.
I think... With the NullObject pattern I should be able to run the app and get a list with the NullStory value.
I'm trying it!
Uhhh...
Reminder: Android emulator doesn't work with Docker running. Pro-tip.
Also; protip - need to wire up the classes to instantiate properly... :\ I was too eager to see if it worked.
It's probably obvious given a moment's thought; but I don't actually crank out this large of a post in a single sitting. I also don't go back and edit (probably much more painfully obvious); but as I was away for a few hours; I was thinking about the Story#title method and how I have it returning a Scanner. Mostly I realized I've got the title represented as a String. It's not a String; it's a title. Guess what's getting a class!
I'll be doing that refactor after I get data displaying.
...
Slowly and not quite TDDing out the VBM for the TopStories screen. The reason it's not quite TDD is that I'm rushing. I lack the pair to bounce ideas off of; so I'm bouncing them off the code. I am refactoring as I go; I am getting tests in and TDDing the process; but it's not as pure as I'd like to be practicing. It doesn't help that I can copy/paste from the already existing MainActivity tests. They too get the top stories; so it's MOSTLY the same.
I think I've got all the required functionality in place... All my tests pass... Let's try the emulator!
Annnnddd.... Nothing displayed. :\
...
I wasted a few days trying to get the list of items displayed. This is where pairing would have been invaluable. I've encountered this type of situation in a pair; takes at most a few hours, not days, to realize the idiocy.
I've not pointed it out earlier in the post; but I lost days, and had epic amounts of frustration because the definition for my item layout had both layout_width and layout_height set to match_parent... The layout_height needs to be wrap_content and hey - Now everything works like I expected it to!!!
Now to undo all the "WTF?!?! WHY?!?!?" changes I made...
OK; back on track. I'm not pulling the actual stories yet; I'm using my NullStory as a placeholder.
...
It looks like some refactoring is required for the Stories class to have the delayed loading of items.
...
I started the refactoring and headed over to a friends to work some more... ... All I can say is push. Remember to push... sigh
...
Back home - back refactoring!
I've been TDDing up the requirements around having a Story refresh when requested.
This starts to required objects to expose a little more information than I'm comfortable with. I think I lack the pattern experience to know better ways. I'm holding it down tight. I hope to find the better ways; but if not... This is an excellent example I can point to and ask "How for this?" and start to learn.
This is part of the reason I'm doing this project. I don't /know/ the ways to resolve a lot of these returned values. I need examples to point to that will help highlight the considerations; not the answers; that go into resolving these issues.
I've done the TDD-ing to get it to the point I can try on the emulator. Check out this commit for the majority of the changes to get the refresh into place. 11/17 files changed to do this were test files.
This is an effect of the practice of ruthless refactoring. Actual tests change less as the API stabilizes. I renamed a method from Stories; and it had cascading effects; but very small. Just API modifications; not rewrites.
It's good to rewrite code to support the new requirements. The more you patch and hack solutions in to avoid changing code - the deeper the technical debt. And why don't you want to rewrite code? Because it might break. How can you be confident it didn't break? You have tests.
Existing Tests are the confidence to change any code you feel will improve the code base.
...
I'm getting an error, NPE to be specific, on a response from trying to get the story data; I've added some logging.
heh - use of the NullStory is biting me here; it's trying to get the details for id=-1.
Little bit of rewriting calls for the correct id.
I went from using the story.storyId() to getting the storyId from the array and providing that.
Wrong
Right
IT WORKS
Aside from a small reminder I will have to handle error conditions
Summary
This post has gone on way too long; and I've beat my head a few too many times. It's all good fun though. I'm making progress! And wow - it worked. Sure I'd get there, but this was awesome.
My biggest takeaway from the work done in this post - Pair. Pairing would have saved so much frustration.