{"action":"created","issue":{"url":"https://api.github.com/repos/spring-cloud/spring-cloud-deployer-cloudfoundry/issues/100","repository_url":"https://api.github.com/repos/spring-cloud/spring-cloud-deployer-cloudfoundry","labels_url":"https://api.github.com/repos/spring-cloud/spring-cloud-deployer-cloudfoundry/issues/100/labels{/name}","comments_url":"https://api.github.com/repos/spring-cloud/spring-cloud-deployer-cloudfoundry/issues/100/comments","events_url":"https://api.github.com/repos/spring-cloud/spring-cloud-deployer-cloudfoundry/issues/100/events","html_url":"https://github.com/spring-cloud/spring-cloud-deployer-cloudfoundry/pull/100","id":185254743,"number":100,"title":"Java Client Code Review","user":{"login":"nebhale","id":60754,"avatar_url":"https://avatars.githubusercontent.com/u/60754?v=3","gravatar_id":"","url":"https://api.github.com/users/nebhale","html_url":"https://github.com/nebhale","followers_url":"https://api.github.com/users/nebhale/followers","following_url":"https://api.github.com/users/nebhale/following{/other_user}","gists_url":"https://api.github.com/users/nebhale/gists{/gist_id}","starred_url":"https://api.github.com/users/nebhale/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nebhale/subscriptions","organizations_url":"https://api.github.com/users/nebhale/orgs","repos_url":"https://api.github.com/users/nebhale/repos","events_url":"https://api.github.com/users/nebhale/events{/privacy}","received_events_url":"https://api.github.com/users/nebhale/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/spring-cloud/spring-cloud-deployer-cloudfoundry/labels/in%20pr","name":"in pr","color":"006b75"}],"state":"open","locked":false,"assignee":{"login":"ericbottard","id":313494,"avatar_url":"https://avatars.githubusercontent.com/u/313494?v=3","gravatar_id":"","url":"https://api.github.com/users/ericbottard","html_url":"https://github.com/ericbottard","followers_url":"https://api.github.com/users/ericbottard/followers","following_url":"https://api.github.com/users/ericbottard/following{/other_user}","gists_url":"https://api.github.com/users/ericbottard/gists{/gist_id}","starred_url":"https://api.github.com/users/ericbottard/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ericbottard/subscriptions","organizations_url":"https://api.github.com/users/ericbottard/orgs","repos_url":"https://api.github.com/users/ericbottard/repos","events_url":"https://api.github.com/users/ericbottard/events{/privacy}","received_events_url":"https://api.github.com/users/ericbottard/received_events","type":"User","site_admin":false},"assignees":[{"login":"ericbottard","id":313494,"avatar_url":"https://avatars.githubusercontent.com/u/313494?v=3","gravatar_id":"","url":"https://api.github.com/users/ericbottard","html_url":"https://github.com/ericbottard","followers_url":"https://api.github.com/users/ericbottard/followers","following_url":"https://api.github.com/users/ericbottard/following{/other_user}","gists_url":"https://api.github.com/users/ericbottard/gists{/gist_id}","starred_url":"https://api.github.com/users/ericbottard/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ericbottard/subscriptions","organizations_url":"https://api.github.com/users/ericbottard/orgs","repos_url":"https://api.github.com/users/ericbottard/repos","events_url":"https://api.github.com/users/ericbottard/events{/privacy}","received_events_url":"https://api.github.com/users/ericbottard/received_events","type":"User","site_admin":false}],"milestone":null,"comments":6,"created_at":"2016-10-25T22:51:58Z","updated_at":"2016-10-26T22:53:08Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/spring-cloud/spring-cloud-deployer-cloudfoundry/pulls/100","html_url":"https://github.com/spring-cloud/spring-cloud-deployer-cloudfoundry/pull/100","diff_url":"https://github.com/spring-cloud/spring-cloud-deployer-cloudfoundry/pull/100.diff","patch_url":"https://github.com/spring-cloud/spring-cloud-deployer-cloudfoundry/pull/100.patch"},"body":"Given some difficulties with the current implementation, I've been reviewing the existing use of the CF Java Client and Reactor to see where improvements can be made.  In pairing, a couple of serious issues were remedied and better idioms were used that should remove a couple classes of issues (primarily around what happens when the API returns 0/1/N results unexpectedly).  Overall though the code was pretty robust.\r\n\r\nI then took an opportunity to refactor the code into a more modular design. Specifically (and I get that this might be controversial), I ensured that all calls to the Java Client were isolated in `requestXXX` methods.  This drastically enlarges the number of methods in the class, but reduces the size of any individual method.  You may also notice that these methods are also `static`.  Both of these points (small methods, `static` methods) are aiming to make any method a \"pure\" function with no side effects in the system.  While there's probably no real benefit in the existing code (outside of composability), it's a really good habit to get into.  It ensures that when it finally comes time to write something that really deserves this style, you'll have it as muscle memory.\r\n\r\nI also took a look at the test code.  Specifically, there were some complaints around mocking the Java Client API.  As we had designed the API for this specific use-case, it was important to understand where the problems lay.  It was clear that as mentioned, there was a huge amount of redundant mocking code.  Going back to the primary code, you'll remember that I isolated the `requestXXX` methods.  One of the major reasons for this is that it identifies all of Java Client APIs that need to be mocked.  Conveniently, even the signatures of \"mock helper\" methods are described by this style as well. You'll see a direct mapping from the `requestXXX` methods to the `givenRequestXXX` with the only change being the addition of a response argument.  Looking at the changes in the tests, the I can see the argument that even these changes aren't an improvement, leaving quite a bit of code (actually more than I started with).  However, I believe that the code now is much more readable with very descriptive names (`givenRequestXXX`) and that the tests are clearer as you read through the mock setup.  That being said, since the properly mocking the tests implies that you understand how the CF server is going to behave and that isn't necessarily true for the poorly spec'd CF APIs, I would totally understand the removal of the unit tests in favor of a pure- integration test style.\r\n\r\nAs a bonus I also added a missing test (`cancel()`)."},"comment":{"url":"https://api.github.com/repos/spring-cloud/spring-cloud-deployer-cloudfoundry/issues/comments/256500639","html_url":"https://github.com/spring-cloud/spring-cloud-deployer-cloudfoundry/pull/100#issuecomment-256500639","issue_url":"https://api.github.com/repos/spring-cloud/spring-cloud-deployer-cloudfoundry/issues/100","id":256500639,"user":{"login":"nebhale","id":60754,"avatar_url":"https://avatars.githubusercontent.com/u/60754?v=3","gravatar_id":"","url":"https://api.github.com/users/nebhale","html_url":"https://github.com/nebhale","followers_url":"https://api.github.com/users/nebhale/followers","following_url":"https://api.github.com/users/nebhale/following{/other_user}","gists_url":"https://api.github.com/users/nebhale/gists{/gist_id}","starred_url":"https://api.github.com/users/nebhale/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nebhale/subscriptions","organizations_url":"https://api.github.com/users/nebhale/orgs","repos_url":"https://api.github.com/users/nebhale/repos","events_url":"https://api.github.com/users/nebhale/events{/privacy}","received_events_url":"https://api.github.com/users/nebhale/received_events","type":"User","site_admin":false},"created_at":"2016-10-26T22:53:08Z","updated_at":"2016-10-26T22:53:08Z","body":"@ericbottard Finished up on our work from this morning.  I even went back and dismantled the `static` usage that you weren't a fan of.  Granted that this needs to run against a real system, but I think this is pretty much ready to go.\r\n\r\n@gregturn I've renamed the `@Before` method back to the non-descriptive `setUp()`."}}
{"actor":{"display_login":"nebhale"}}