Code has OOP smells but I can't think of a better refactor

[randomshi...@gmail.com]

First - I have a Node struct, which represents a network connection to a node.

type Node struct {
    *apiclient.Node
}
func (c *Node) GetStatus() (status *models.Status, err error)
func (c *Node) PostTransaction(signedEncodedTx, signedEncodedTxHash string) (err error)
func (c *Node) GetTopBlock() (kb *models.KeyBlockOrMicroBlockHeader, err error)
func (c *Node) GetHeight() (height uint64, err error)
func (c *Node) GetCurrentKeyBlock() (kb *models.KeyBlock, err error)
func (c *Node) GetAccount(accountID string) (account *models.Account, err error)
func (c *Node) GetNameEntryByName(name string) (nameEntry *models.NameEntry, err error)
func (c *Node) GetGenerationByHeight(height uint64) (g *models.Generation, err error)
func (c *Node) GetMicroBlockTransactionsByHash(microBlockID string) (txs *models.GenericTxs, err error)
func (c *Node) GetMicroBlockHeaderByHash(microBlockID string) (txs *models.MicroBlockHeader, err error)
func (c *Node) GetKeyBlockByHash(keyBlockID string) (txs *models.KeyBlock, err error)
func (c *Node) GetTransactionByHash(txHash string) (tx *models.GenericSignedTx, err error)
func (c *Node) GetOracleByPubkey(pubkey string) (oracle *models.RegisteredOracle, err error)
func (c *Node) GetOracleQueriesByPubkey(pubkey string) (oracleQueries *models.OracleQueries, err error)
func (c *Node) GetContractByID(ctID string) (contract *models.ContractObject, err error)

I want to be able to mock out the node's methods, so for every Node method, I make an interface.

type GetStatuser interface
type PostTransactioner interface
type GetTopBlocker interface
type GetHeighter interface
type GetAccounter interface
type GetNameEntryByNamer interface
type GetGenerationByHeighter interface
type GetMicroBlockTransactionsByHasher interface
type GetMicroBlockHeaderByHasher interface
type GetKeyBlockByHasher interface
type GetTransactionByHasher interface
type GetOracleByPubkeyer interface

Sometimes, I just need to return a Node instance. However, to make that code testable without an actual Node connection, I have to introduce this large interface, which is only used once.

type NodeInterface interface {
    GetAccounter
    GetTopBlocker
    GetStatuser
    GetHeighter
    GetKeyBlockByHasher
    GetOracleByPubkeyer
    GetGenerationByHeighter
    GetTransactionByHasher
    GetNameEntryByNamer
    GetMicroBlockHeaderByHasher
    GetMicroBlockTransactionsByHasher
    PostTransactioner
}

Some functions that build upon Node methods are used quite often, so I put them in a Helper struct to avoid having to specify the Node instance every time.

Again, to make code above independent from Helpers or an actual Node connection, I have to introduce a large interface.

type Helpers struct {
    Node GetHeightAccountNamer
}
func (h Helpers) GetTTL(offset uint64) (ttl uint64, err error)
func (h Helpers) GetNextNonce(accountID string) (nextNonce uint64, err error)
func (h Helpers) GetTTLNonce(accountID string, offset uint64) (height uint64, nonce uint64, err error)
func (h Helpers) getAnythingByName(name string, key string) (results []string, err error)
func (h Helpers) GetAccountsByName(name string) (addresses []string, err error)
func (h Helpers) GetOraclesByName(name string) (oracleIDs []string, err error)
func (h Helpers) GetContractsByName(name string) (contracts []string, err error)
func (h Helpers) GetChannelsByName(name string) (channels []string, err error)
type HelpersInterface interface {
    GetTTL(offset uint64) (ttl uint64, err error)
    GetNextNonce(accountID string) (nextNonce uint64, err error)
    GetTTLNonce(accountID string, offset uint64) (height uint64, nonce uint64, err error)
    GetAccountsByName(name string) (addresses []string, err error)
    GetOraclesByName(name string) (oracleIDs []string, err error)
    GetContractsByName(name string) (contracts []string, err error)
    GetChannelsByName(name string) (channels []string, err error)
}

The functions that use the Helper struct are always executed in the context of a particular address and node, so I make a Context struct that combines Helpers and an Address.

// Context stores relevant context (node connection, account address) that one might not want to spell out each time one creates a transaction
type Context struct {
    Address string
    Helpers HelpersInterface
}
func (c *Context) SpendTx(senderID string, recipientID string, amount, fee big.Int, payload []byte) (tx SpendTx, err error)
func (c *Context) NamePreclaimTx(name string, fee big.Int) (tx NamePreclaimTx, nameSalt *big.Int, err error)
func (c *Context) NameClaimTx(name string, nameSalt big.Int, fee big.Int) (tx NameClaimTx, err error)
func (c *Context) NameUpdateTx(name string, targetAddress string) (tx NameUpdateTx, err error)
...

Problems:
1. After using the `Context` methods to create a Transaction, it is necessary to `node.BroadcastTransaction()`. However, because of `HelpersInterface`, the code above cannot access the underlying Node instance.
So I either create a new Node above, or return one from `Context`'s constructor

func NewContextFromURL(url string, address string, debug bool) (ctx *Context, node *Node)

which seems inelegant.

Of course I could add

Helpers.BroadcastTransaction()

but unlike the other helper methods, BroadcastTransaction() does not need any extra code around it.

2. After writing the question it seems like eliminating the Helpers struct will solve the problem. But I don't know if it is really THE solution, because I used the same pattern with NodeInterface, and sometimes I really do need to say that I will pass in a full-featured Node, so I have to have this large interface.

3. Somehow I feel like the whole idea of making code easier to write by putting frequently used variables into a class/struct (yes I know Go has no classes) and calling the methods of that struct is not how Go is supposed to be used, but I am not sure.

[burak serdar]

These are my opinions, and others may disagree.

Up until now, I think what you did is tedious but acceptable. However,
I wouldn't write the Helpers struct, especially not for the reason you
mentioned. All these functions can either be written for Node, or they
can be standalone functions that take a Node parameter (or
NodeInterface). I don't think there is a need for a helpers interface.

Context would make sense if you had a Node (or NodeInterface) instead
of Helpers.

> ...
>
>
>
>
>
> Problems:
> 1. After using the `Context` methods to create a Transaction, it is necessary to `node.BroadcastTransaction()`. However, because of `HelpersInterface`, the code above cannot access the underlying Node instance.
> So I either create a new Node above, or return one from `Context`'s constructor

Get rid of Context.Helpers, instead use Context.Node.

>
> func NewContextFromURL(url string, address string, debug bool) (ctx *Context, node *Node)
>
>
>
> which seems inelegant.
>
> Of course I could add
> Helpers.BroadcastTransaction()
>
> but unlike the other helper methods, BroadcastTransaction() does not need any extra code around it.
>
> 2. After writing the question it seems like eliminating the Helpers struct will solve the problem. But I don't know if it is really THE solution, because I used the same pattern with NodeInterface, and sometimes I really do need to say that I will pass in a full-featured Node, so I have to have this large interface.

You should consider simply using Node in your code. If testing is a
concern and you cannot mock it, you can use NodeInterface.

>
> 3. Somehow I feel like the whole idea of making code easier to write by putting frequently used variables into a class/struct (yes I know Go has no classes) and calling the methods of that struct is not how Go is supposed to be used, but I am not sure.

That's not the purpose of structs. You should treat structs as data
types, with some cohesion in its fields and methods. It is not just a
collection of fields/methods.

>
> --
> You received this message because you are subscribed to the Google Groups "golang-nuts" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/3ee266e3-ccb4-401c-a754-addbda544349%40googlegroups.com.

[Shinichi Kudo]

Thank you! In the end I forgot that I had introduced the Helpers because
I needed a higher level mocking point for something that mostly won't
use Context - but then I found a powerful new tool, closures. Those
solved the elegance problem completely.

Andrew Chiw