A philosophy I like and have made a(n unknowingly) passive effort to implement before is called "Clean Architecture". It's the idea of an application having dependencies that flow "outward".
Go read The Clean Architecture by Uncle Bob; for the full picture. A simple way to state this is, "No Circular Dependencies". If A depends on B; B and nothing B depends on can have a dependency on A. It forces the application to be less complex. It prevents changes from circling back around to the class that originally changed.
I've had discussions with other programmers, and not all feel strongly about this. I do. It's something I've tried to do for a few years. It's the Layers design pattern enforced.
Not "tried" in the sense of knowing what I was doing, but tried in the sense of "layering" my design, which prevented most of the cyclical dependency graphs. Because I wasn't really aware of what I was flirting about with; I didn't get it well enforced.
Now that I know - I want to enforce it strongly.
I see issues in the HackerNews App.
When I first created this topic to work on I made a few notes for myself; largely that the Strategy pattern I've employed between the Bridge and Mediator layers has them tightly coupled to each other.
This is an instance where I've allowed myself to talk ... my...self out of good design. "It's all the 'UI' layer, it's OK".
I recall a recent post about a RecyclerView that was all 'UI' and I thought it would be 'OK' to not have it tested... Yea... That didn't work then; it's not working now. The code was telling me what it wanted... I didn't listen. I've found a good design practice that I want to implement... and lo-and-behold; the code was trying to get me here.
From this; I surmise that reading the hell out of the literature is going to help listen to the code. I've had a number of instances, both in this HackerNews app and at work where being more familiar with some practices (and less so Design Patterns) would have reduced the churn and allowed progress to be made much sooner.
It's not a huge hit, doing TDD makes refactoring a breeze. I'd like to get these things in round one though; actually, listen to the code when it's trying to help me.
I'm getting better; every mistake helps me hear and act in the future.
Making Changes
As mentioned; I wasn't sure of how to decouple the Strategy I had in place between the TopStoriesActivityBridge and TopStoriesActivityMediator. I'd tried a few things in the past, but it always spun around in circles. It wanted to be decoupled; I could hear its cries for help - I just couldn't see how.
Any efforts I made at that point would just hurt the system more. The code fought the changes I was trying, and until I could find what the code wanted; I didn't do anything. I think this is the best approach. Don't force the code to do something; if it fights; wait until you realize what it already knows.
I'm kinda thinking that, as a software engineer, it's less about making a computer do what I want, and helping the computer do what it wants.
Like a sculptor who brings out the sculpture hidden in the stone, we bring out the program hidden in the.. uhh... Apologies to Michelson–Morley; I'm bringing back the aether!
Yeah, yeah - That's all poetic and crap. BORING... back to tech.
Solving the Coupling
I did a spike to find a form that decoupled the classes and supported a Clean Architecture between the UI layers.
It wasn't "hard". It took me about 20 minutes of focused time. Before this; I was always trying to do it as an ancillary activity to some other task. That part of the code felt wrong; I made a quick attempt to fix (RUTHLESS REFACTOR!!!) and was unable to get it solved, so I returned to my focused task.
Today my focus is getting clean architecture in place. I spiked to find a way to clean it up. I found it.
Then I deleted it.
git reset --hard
What I had wasn't Test Driven. It got thrown out. I know it can happen, I know the rough direction to take; I don't need all the details that existed from the spike. Really; I don't want them.
The only piece I'm mentally going to follow is putting an interface in the TopStoriesActivityMediator that the TopStoriesActivityBridge will implement... ya know... Clean Architecture.
Let's get started - Mediator's Bridge
The interface is the representation of all the interactions that TopStoriesActivityMediator needs with the TopStoriesActivityBridge. I don't use a general Mediator and Bridge because each will have its own interactions. I'm not going to try and force a generic interface at this point. Well - I'm hopefully never going to force a generic interface. If such a generic form emerges; then yes, I'll tend it and get it in place; but I have no expectation of genericizing the interactions of these layers.
What interactions do we have from mediator to bridge (I'm short handing, not genericizing)? There are 3; when an item is updated, when data is to be loaded, and when there's an error loading data.
Sounds like 3 methods for an interface.
I'm going to tackle the Data Error case first.
I'm creating the test
@Test
public void TopStoriesBridgeShouldProvideDataError(){
}
which I'm not a fan of the name. Oh well; let's run with it. I'm using this to drive the creation... Screw it - I'm going with
@Test
public void TopStoriesBridgeExists{
}
which drives the creation of the interface
@Test
public void TopStoriesBridgeExists(){
new TopItemsActivityMediator.TopStoriesBridge(){};
}
I find myself wondering how to test an interface... I ... can't? Not really. I can drive the creation of the interface itself and the method declarations... but even those tests are fragile. They don't test functionality; they test, essentially - implementation.
So... Nope.
DELETED!
coughinterface creation was still TDDdcough
From here; I feel like I'm missing the TDD of this...
Let's think - The simplest test I could write - Create the DataError functionality decoupled. Since it's currently implemented just exposing a run method... Let's do it as a Runnable? Can always add a class later if required.
@Test
public void createDataErrorRunnable(){
}
It's a test - Let's fill it.
Since we're looking to replace the DataStrategy; I'm just pulling the core of the test from there into the above test.
@Test
public void createDataErrorRunnable() throws InterruptedException {
final CountDownLatch dialogLatch = new CountDownLatch(1);
final FakeDialogBuilder fakeDialogBuilder = new FakeDialogBuilder(dialogLatch);
AlertDialogBuilderAccess.setActiveDialogBuilder(fakeDialogBuilder);
final TopStoriesActivityBridge topStoriesActivityBridge = simpleBridge();
topStoriesActivityBridge.dataError().run();
assertThat(dialogLatch.await(1, TimeUnit.SECONDS)).isTrue();
fakeDialogBuilder.assertShowCalled(1);
}
And the test fails!
Of course, I haven't implemented the code in the Runnable so... yea. Failure.
Let's go implement things!
A little copy paste from the existing Strategy
/* package */ Runnable dataError() {
return new Runnable() {
@Override
public void run() {
new AlertDialogBuilder<>()
.init(TopStoriesActivityBridge.this.topStoriesActivity)
.setTitle(R.string.top_stories_strings_alert_dialog_failure_title)
.setMessage(R.string.top_stories_strings_alert_dialog_failure_message)
.show();
}
};
}
sets us up pretty good. Let's try the test!
GREEN!!!
Which means we've correctly implemented it; well - the migrated test passes - Which is enough confidence for me.
Next step is to implement the DataLoad replacement. I'd like to migrate everything to use the new dataError first, but there'd be two types... it'd be ugly. We'll be satisfied with the one replacement, add DataLoad then migrate the next piece.
@Test
public void createDataLoadRunnable() throws InterruptedException{
}
A simple enough starting place; just like the other; get an instance of TopStoriesActivityBridge in place to drive the development of the method from.
One thing I'm doing SLIGHTLY different from the dataError test;
@Test
public void createDataChangedRunnable() throws InterruptedException{
final FakeTopStoriesActivity fakeTopStoriesActivity = new FakeTopStoriesActivity(null);
final TopStoriesActivityBridge topStoriesActivityBridge = new TopStoriesActivityBridge(fakeTopStoriesActivity);
final Runnable dataLoad = topStoriesActivityBridge.dataLoad();
}
is having the method result set into a variable vs calling run() off of it.
The reason for this is that when I use the tool to create the method; it knows the type. I've gotta do nothing more for the signature.
Migrating the teeny code from the previous DataChanged test gives us
@Test
public void dataChangedShouldLoadChangedData() throws InterruptedException{
final TopStoriesActivity mockTopStoriesActivity = Mockito.mock(TopStoriesActivity.class);
final TopStoriesActivityBridge topStoriesActivityBridge = new TopStoriesActivityBridge(mockTopStoriesActivity);
final Runnable dataLoad = topStoriesActivityBridge.dataLoad();
Mockito.doNothing().when(mockTopStoriesActivity).notifyTopStoriesChanged();
dataLoad.run();
Mockito.verify(mockTopStoriesActivity).notifyTopStoriesChanged();
}
Which passes - so... YAY!
OK, now to migrate the mediator.
...
Which was far less than I thought... but that's been happening a few times in these refactorings. It's just... less work. Maybe it's cause I don't have to think about it liked I'd have to before. Just... Make changes, run tests - Green? Green. Move to next refactor.
Also the simpler implementation - easier to refactor quickly.
We have one more refactor to make between these two.
Currently, I only have two "strategies" but there's a third interaction between the mediator and bridge that needs to be abstracted.
/* package */ Item itemAt(final int index) {
if (topItems == null) { return Item.NullItem; }
return topItems.itemAt(index, topStoriesActivityBridge::notifyTopStoryChanged);
}
We have dataError and dataChanged, now we need itemChanged.
Unlike the previous functionality that got refactored; there's not a strategy for this. I did do it in my spike... I just don't remember exactly how. Which is why I deleted it. It needs to be TDD.
What I think I'll do before I plug that in is to add the interface that the Bridge will implement to be for the Mediator.
When I have something like this
new TopItemsActivityMediator(new TopStoriesActivityBridge(new TopStoriesActivity()));
in a test; I should REALLY know that I'm doing something wrong. It's kinda smelly.
Interface implemented.
The smell now looks like
new TopItemsActivityMediator(Mockito.mock(TopItemsActivityMediator.Bridge.class));
The TopStoriesActivityMediatorTests class is going to get a huge refactor to use a mocked interface vs the Fake's I currently have.
There's a test I started working on
public void itemAtShouldReturnStory() throws InterruptedException {
final HackerNewsNetworkTestResponses.Builder builder = new HackerNewsNetworkTestResponses.Builder();
hackerNewsNetworkTestResponses.simpleItemIdList(mockWebServer, builder);
final Item simpleStory = hackerNewsNetworkTestResponses.simpleStory(mockWebServer, builder);
final CountDownLatch latch = new CountDownLatch(1);
final TopItemsActivityMediator mediator = new TopItemsActivityMediator(
new FakeTopStoriesActivityBridge(new FakeTopStoriesActivity(latch),
Mockito.mock(TopItemsAdapter.class)));
mediator.loadTopStoriesData();
assertThat(latch.await(1, TimeUnit.SECONDS)).isTrue();
assertThat(mediator.itemAt(0)).isEqualTo(NullItem);
Thread.sleep(500);//Sleeping so the fake network can update
final Item actualItem = mediator.itemAt(0);
assertThat(actualItem).isEqualTo(simpleStory);
assertThat(actualItem.isStory()).isTrue();
}
and after I made changes I was red for more than 15 minutes.
I did a hard reset to when the test was green.
Time to try again. I think I was doing too many changes at once. Let's see if I can make smaller steps this time.
The first thing I'm doing is straight replacing
final TopItemsActivityMediator mediator = new TopItemsActivityMediator(new FakeTopStoriesActivityBridge(new FakeTopStoriesActivity(latch), Mockito.mock(TopItemsAdapter.class)));
with
final TopItemsActivityMediator mediator = new TopItemsActivityMediator(mockBridge);
This simple change fails the test.
There's a null response in the TopItemsActivityMediator
dataLoadStrategyFactory(response).run();
This should be returning the dataChanged runnable; so we can set the mock to return a runnable when dataChanged is called.
Which I'll have done nothing for now. The test calls for nothing more at this point.
Running the tests again; it fails with the latch not having counted down. Checking the pre-change test; it was counting down in the activities notifyTopStoriesChanged method.
Sounds like a nice override for our interface's notifyTopStoryChanged.
It's still failing on the latch assert.
This is pretty much where I was the last time. I thrashed a lot on this.
I'm going to take a slightly different approach this time.
I'm going to tell you what I'm thinking.
There are a couple network calls that take place. The first is to get the list of ID's; then the ID's are updated.
When an item is requested; if it hasn't been loaded; will make a background request for the item details.
This does not appear to be happening from the initial "NullObject" check (pre the 500 sleep).
The latch.await isn't triggering. Which implies I need to move it to the run... this feels familiar. BUT - Typing it out might be helping. I'm thinking of paths I didn't realize before.
I want to point out that I'm trying to find where I need to update in the test to KEEP the latch.await instead of just removing it. It was needed before; it should be needed now; at least it should fit into the flow now - just... where.
Being in the run has gotten us a passing test.
Everything we had before is still in place; we just changed how we controlled the flow so the threading could operate.
Now I can refactor and try to clean up the test. The sleep is really troublesome.
I'm adding in a block
Mockito.doAnswer(new Answer() {
@Override
public Object answer(InvocationOnMock invocation) throws Throwable {
itemLatch.countDown().
return null;
}
}).when(mockBridge).notifyTopStoryChanged(anyInt());
The itemLatch is a separate latch to block until the item is sucessfully loaded.
This will remove the sleep, and provide better test performance.
This passes!
The test takes ~200ms where before it took ~700ms.
removing the 500ms sleep saved; unsurprisingly, 500ms.
The tests run faster; this is awesome!
Kinda ruins my plans for a post on improving test speed... I might do that still, just a shift from working to improve performance, to recounting ways I have improved performance... and yes... "Don't use Thread.sleep was going to be one of them".
I copy/pasted a chunk of the test we just updated into the next test down (verifies job vs story); now I've extracted it.
There's no reason to let that sit as duplicate code. I'll let it stay if I've typed it in, but a C&P; just methodize it.
Next up
Bridge's Activity Interface
The bridge will interact with the activity. The activity is an outer (an outermost) layer in the architecture, so the Bridge must not know about it.
The Activity needs to implement the interface of the layer(s) below. You know this; as it's exactly what we did for the Bridge and Mediator.
I haven't examined this one before; or much? for tight coupling making the Clean Architecture difficult. Since I seem to have seen nothing; I'm hoping it'll be straight forward.
First thing though; we're going to finalize the renaming of these classes from TopStories to TopItems.
Renamed... and I should have run the tests after each... It's hard sometimes.
A constructor test (exception text) failed. Updated the expected text and all is good.
The rename refactor doesn't do test text well it seems.
Checking the Bridge code; there are three instances the activity is used
@Override
public Runnable dataError() {
return () -> new AlertDialogBuilder<>()
.init(TopItemsActivityBridge.this.topItemsActivity)
.setTitle(R.string.top_stories_strings_alert_dialog_failure_title)
.setMessage(R.string.top_stories_strings_alert_dialog_failure_message)
.show();
}
@Override
public Runnable dataChanged() {
return TopItemsActivityBridge.this.topItemsActivity::notifyTopStoriesChanged;
}
@Override
public void notifyTopStoryChanged(final int index){
topItemsActivity.notifyTopStoryChanged(index);
}
The first one is a context. I haven't thought this one through all the way yet.
I don't want to expose things as an activity if it's not required. It only requires a context... so... as a context it is.
We'll get there. For now; create the View interf... View's a thing. OK; create the Display ... also a thing...
UiView - HA! ... OK; create the constructor that takes the UiView. I hate that name, I really do. I lack better ATM; so it sticks.
Ahh - I see the trouble I had at the gym making the transition for the bridge/mediator. I think I stumbled into it, but working on the bridge/view it became crystal clear. I'm not modifying the Activity. I'm updating the Bridge to take the interface instead of the activity class. Then I update the Activity to ... work.
Sure sure, obvious. I know - Duh! I'm so dumb. I kept wanting to update both the Activity and Bridge. That's a pretty big bite; hehe.
TopItemsActivityBridge now takes only the UiView.
Interface or Abstract Class
I'm considering making these abstract classes instead of interfaces. The reason for this is that the method of an interface is forced public; where the abstract class can keep the methods package-protected.
I dislike the exposure.
I'm gonna see what it looks... shit can't. It has to be an interface. The activity already extends a class! duh... Whelp... Guess... At this level; if developers are trying to do stupid things; the entire source code is available to do stupid with.
If it was a library; I might do some less-than-ideal-code to hide information; but with this project, it's not appropriate. It would be a bad example. ... I think. I'll assume so for now.
Summary
It wasn't a lot of work to refactor to a clean architecture. I think a large part of that is how I approach design. As mentioned early in the post; the "Clean Architecture" is largely how I approach a design. Especially with some recent personal emphasis on ensuring no cyclic dependencies.
In the one place, I knew there'd be a little issue (as I'd failed at weak attempts before) I spiked and found a solution - just had to then go through it TDD.
The second set of changes showed that I wasn't changing two classes by adding the interface; just one class to take the interface, and eventually, a failing test would force it to be implemented.
For funzies - Let's run the emulator!
Test failure?!?!?
runs tests Passes
Uhh.... Huh; the thread is different; which I have hard coded in the FyzLog tests.
Modifying that to pull the current thread name.
And it doesn't run. There was an oddity that I noticed while making the updates; but since the tests passed... But that means I'm missing a test.
I'm very missing a test. I put in a breakpoint and... it never got hit. Hmmm... It has to be getting hit...
It was using a method reference
return topItems.itemAt(index, bridge::notifyTopStoryChanged);
I expand it out to
return topItems.itemAt(index, new Items.ItemRefreshCallback() {
@Override
public void itemRefreshed(final int index) {
bridge.notifyTopStoryChanged(index);
}
});
and code coverage has it covered.
But I'm getting an exception that the object passed in doesn't implement the expected interface.
So... There's a bug in the Jack Compiler. Given that it's going away - I won't care - but... hmmmm...
When I change to the second form, with new it runs.
Let's change it back and see.
Well... not change it back, but use the tool to get to a few intermediate forms.
I'm going to go to the lambda form first
return topItems.itemAt(index, (int index1) -> bridge.notifyTopStoryChanged(index1));
Loads.
Back to method reference
return topItems.itemAt(index, bridge::notifyTopStoryChanged);
And now it works...
I hate computers.
That is all for now; I've decoupled my UI layer for the Clean Architecture.
I don't know any other pieces that lack the Clean Architecture; but if I find it, I'll work on cleaning it.