REFACTOR: More information of what to refactor #1

New Issue

fenpaws · 2023-11-15T14:50:20Z

fenpaws commented

2023-11-15 14:50:20 +00:00

Hi @daskadse,

I'm working on refactoring the SDK, there is a branch called refactor where I already started to work on it. You mentioned having some ideas/insights, and I'd love to hear them.

Could you share a list of things to change or tasks to tackle? Your input would be helpful in making sure we're on the right track.

Thanks a bunch!

Hi @daskadse, I'm working on refactoring the SDK, there is a branch called [refactor](https://git.dragse.it/anthrove/e621-sdk-go/src/branch/refactor) where I already started to work on it. You mentioned having some ideas/insights, and I'd love to hear them. Could you share a list of things to change or tasks to tackle? Your input would be helpful in making sure we're on the right track. Thanks a bunch!

fenpaws added the

enhancement

help wanted

labels 2023-11-15 14:50:20 +00:00

fenpaws added this to the Anthrove project 2023-11-15 14:50:26 +00:00

fenpaws self-assigned this 2023-11-15 14:50:31 +00:00

fenpaws removed this from the Anthrove project 2023-11-15 14:50:37 +00:00

daskadse commented

2023-11-15 20:11:34 +00:00

My main gripe is the inconsistency in the high level API design. Some functions directly request the endpoint (to be fair, these are in implementation just now):

pools, err := client.GetLatestPoolsDBExportDataAsStruct()

Some don't do any request, but instead return the builder which has to be executed:

builder := client.GetPostByID(12345)
post, err := builder.Execute()

Some actually do do a request, but you wouldn't know because you still get a builder returned:

builder, err := client.GetFavoritesForUser("foo") // <- actually does a request!

And then there is the ambiguity of getting a builder for paginated content (like favorites) which can be executed just like any other request builder, but only results in getting results from the first page:

builder, err := client.GetFavoritesForUser("selloo")
favs, err := builder.Execute() // <- only gets the first page

Apart from that, there's some pretty poor naming, like GetFavoritesForUser being a builder factory and GetNFavoritesForUser being a builder consumer, which is in no way clear. It also seems a little counter-intuitive that you need to call the method on client again (I know the architectural reason for this, but I don't like it).

For a nice, coherent API, the pattern used should be consistent. That can be a builder pattern, but would look something more like this:

favs1, err := client.RequestFavorites().FromUsername("selloo").WithTags("male", "knot").Limit(100).Execute()

favs2, err := client.RequestFavorites().FromUserID(1234).Execute() // gets all favs by default

In fact, I think the builder pattern is pretty good for this, considering its whole point is to collapse many possible function definitions because of alternative arguments into one readable statement. However, this would architecturally require yet another full builder implementation for most types of request for the high level API.

The alternatives would be to either use different functions which do the requests directly (which would need to be a lot), immediately returning the results:

favs1, err := client.RequestNFavoritesFromUserWithTags(100, "selloo", []string{"male", "knot"})

favs2, err := client.RequestAllFavoritesFromUserId(1234)

Or to work with optional arguments via variadic functions which contain optional additional configuration:

// func RequestFavoritesFromUser(username string, optargs ...OptionalFavArgs) ([]model.Post, error)

favs1, err := client.RequestFavoritesFromUser("sellooo", OptionalFavArgs { limit: 100, tags: []string{"male", "knot"} })

favs2, err := client.RequestFavoritesFromUserId(1234) // gets all favs by default

Issue is that this only works for optional arguments, not for either/or arguments (like username/userID) and that readability is in my eyes worse than with a builder pattern. It is however still a lot better than exploding it all into separate functions and is less work to implement than another layer of builder pattern. Also, compared to the builder pattern, it is technically stricter at compile time, except if you create the builder with type state, but that would be hell in go, so it's not really an option. Therefore, you'd totally be able to create invalid state with the builder which would only error at runtime, which is not the case with optional arguments (at least not if any combination of them is allowable).

My main gripe is the inconsistency in the high level API design. Some functions directly request the endpoint (to be fair, these are in implementation just now): ```go pools, err := client.GetLatestPoolsDBExportDataAsStruct() ``` Some don't do any request, but instead return the builder which has to be executed: ```go builder := client.GetPostByID(12345) post, err := builder.Execute() ``` Some actually _do_ do a request, but you wouldn't know because you still get a builder returned: ```go builder, err := client.GetFavoritesForUser("foo") // <- actually does a request! ``` And then there is the ambiguity of getting a builder for paginated content (like favorites) which can be executed just like any other request builder, but only results in getting results from the first page: ```go builder, err := client.GetFavoritesForUser("selloo") favs, err := builder.Execute() // <- only gets the first page ``` Apart from that, there's some pretty poor naming, like `GetFavoritesForUser` being a builder factory and `GetNFavoritesForUser` being a builder consumer, which is in no way clear. It also seems a little counter-intuitive that you need to call the method on `client` again (I know the architectural reason for this, but I don't like it). --- For a nice, coherent API, the pattern used should be consistent. That _can_ be a builder pattern, but would look something more like this: ```go favs1, err := client.RequestFavorites().FromUsername("selloo").WithTags("male", "knot").Limit(100).Execute() favs2, err := client.RequestFavorites().FromUserID(1234).Execute() // gets all favs by default ``` In fact, I think the builder pattern is pretty good for this, considering its whole point is to collapse many possible function definitions because of alternative arguments into one readable statement. However, this would architecturally require yet another full builder implementation for most types of request for the high level API. The alternatives would be to either use different functions which do the requests directly (which would need to be _a lot_), immediately returning the results: ```go favs1, err := client.RequestNFavoritesFromUserWithTags(100, "selloo", []string{"male", "knot"}) favs2, err := client.RequestAllFavoritesFromUserId(1234) ``` Or to work with optional arguments via variadic functions which contain optional additional configuration: ```go // func RequestFavoritesFromUser(username string, optargs ...OptionalFavArgs) ([]model.Post, error) favs1, err := client.RequestFavoritesFromUser("sellooo", OptionalFavArgs { limit: 100, tags: []string{"male", "knot"} }) favs2, err := client.RequestFavoritesFromUserId(1234) // gets all favs by default ``` Issue is that this only works for optional arguments, not for either/or arguments (like username/userID) and that readability is in my eyes worse than with a builder pattern. It is however still a lot better than exploding it all into separate functions and is less work to implement than another layer of builder pattern. Also, compared to the builder pattern, it is technically stricter at compile time, except if you create the builder with type state, but that would be hell in go, so it's not really an option. Therefore, you'd totally be able to create invalid state with the builder which would only error at runtime, which is not the case with optional arguments (at least not if any combination of them is allowable).

fenpaws changed title from ~~REFCTOR: More information of what to refactor~~ to REFACTOR: More information of what to refactor

2023-11-18 15:11:36 +00:00

This repo is archived. You cannot comment on issues.