{"action":"created","issue":{"url":"https://api.github.com/repos/bitcoinj/bitcoinj/issues/344","labels_url":"https://api.github.com/repos/bitcoinj/bitcoinj/issues/344/labels{/name}","comments_url":"https://api.github.com/repos/bitcoinj/bitcoinj/issues/344/comments","events_url":"https://api.github.com/repos/bitcoinj/bitcoinj/issues/344/events","html_url":"https://github.com/bitcoinj/bitcoinj/pull/344","id":66072865,"number":344,"title":"Refactor The TransactionBag Dependency in Transaction.java; Improve TransactionTest coverage","user":{"login":"haxwell","id":3296972,"avatar_url":"https://avatars.githubusercontent.com/u/3296972?v=3","gravatar_id":"","url":"https://api.github.com/users/haxwell","html_url":"https://github.com/haxwell","followers_url":"https://api.github.com/users/haxwell/followers","following_url":"https://api.github.com/users/haxwell/following{/other_user}","gists_url":"https://api.github.com/users/haxwell/gists{/gist_id}","starred_url":"https://api.github.com/users/haxwell/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/haxwell/subscriptions","organizations_url":"https://api.github.com/users/haxwell/orgs","repos_url":"https://api.github.com/users/haxwell/repos","events_url":"https://api.github.com/users/haxwell/events{/privacy}","received_events_url":"https://api.github.com/users/haxwell/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":3,"created_at":"2015-04-03T04:30:02Z","updated_at":"2015-04-07T03:07:37Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/bitcoinj/bitcoinj/pulls/344","html_url":"https://github.com/bitcoinj/bitcoinj/pull/344","diff_url":"https://github.com/bitcoinj/bitcoinj/pull/344.diff","patch_url":"https://github.com/bitcoinj/bitcoinj/pull/344.patch"},"body":"I moved all methods that required a TransactionBag parameter from Transaction to TransactionUtils. I believe this is a more correct placement because when the method is in Transaction, it requires a TransactionBag (ie: a Wallet) to be passed in. So, for TransactionTest, that means a mock. Also, Wallet is the only code that called these methods. So they all support/enhance Wallet's functionality, and not Transaction's. Transaction is not, then, the best place for them.\r\n\r\nMy original purpose sprung from trying to remove the mocks necessary to test Transaction.isConsistent(). But that mock would always be required, as a TransactionBag of some sort must be passed in to exercise that method's code. I did, however, notice that the implementation of the method did not depend on any private/protected parts of the Transaction object. And further it was only called by Wallet. It could be moved to a neutral location (along with the mock necessary to test it) and Wallet could call it there, keeping all functionality it had, and the mock presently in TransactionTest.isConsistent() would be gone.\r\n\r\nThere's more to say, but I will let the code talk... I've been in my head on this for days.. do let me know what you think.. I think this is the right move, but it may need some polish."},"comment":{"url":"https://api.github.com/repos/bitcoinj/bitcoinj/issues/comments/90331824","html_url":"https://github.com/bitcoinj/bitcoinj/pull/344#issuecomment-90331824","issue_url":"https://api.github.com/repos/bitcoinj/bitcoinj/issues/344","id":90331824,"user":{"login":"haxwell","id":3296972,"avatar_url":"https://avatars.githubusercontent.com/u/3296972?v=3","gravatar_id":"","url":"https://api.github.com/users/haxwell","html_url":"https://github.com/haxwell","followers_url":"https://api.github.com/users/haxwell/followers","following_url":"https://api.github.com/users/haxwell/following{/other_user}","gists_url":"https://api.github.com/users/haxwell/gists{/gist_id}","starred_url":"https://api.github.com/users/haxwell/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/haxwell/subscriptions","organizations_url":"https://api.github.com/users/haxwell/orgs","repos_url":"https://api.github.com/users/haxwell/repos","events_url":"https://api.github.com/users/haxwell/events{/privacy}","received_events_url":"https://api.github.com/users/haxwell/received_events","type":"User","site_admin":false},"created_at":"2015-04-07T03:07:37Z","updated_at":"2015-04-07T03:07:37Z","body":"You know, I did think about that.. I should have listened to my self.. :)\r\n\r\nRegarding why the new class, my thought was that yeah, Wallet is the only thing using those methods now, but the methods are all centered around TransactionBag, so some other implementation of TransactionBag may come around (one that didn't inherit from Wallet. It could happen), and we'd have the same problem.\r\n\r\nAnd I originally deprecated the methods, and then called the new class, rather than deleting them. But then I thought, fuck it. Code Boldly. and I guess I got a bit carried away with Ctrl-D. Note to self!\r\n\r\nSo, I guess since the refactor is basically (justly) denied, then we're back to #341, yes? All the changes you had requested were made in the most recent iteration of that PR."}}