From 0193dac1ebc2de2b79de907a91ff5020c8403edc Mon Sep 17 00:00:00 2001 From: Arran Hobson Sayers Date: Fri, 19 Apr 2024 14:49:52 +0100 Subject: [PATCH] Improve search again --- .golangci.yaml | 6 +++ goodreads/client.go | 93 ++++++++++++++++++++++------------------ goodreads/client_test.go | 7 +-- server/server.go | 4 +- utils/utils.go | 28 ------------ 5 files changed, 60 insertions(+), 78 deletions(-) delete mode 100644 utils/utils.go diff --git a/.golangci.yaml b/.golangci.yaml index 1f31fdb..4f37a3d 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -25,6 +25,12 @@ linters-settings: - name: cognitive-complexity arguments: [15] + - name: cyclomatic + arguments: [15] + + - name: empty-lines + disabled: true + - name: line-length-limit arguments: [120] diff --git a/goodreads/client.go b/goodreads/client.go index cf92d85..596a867 100644 --- a/goodreads/client.go +++ b/goodreads/client.go @@ -12,7 +12,7 @@ import ( "strconv" "sync" - "github.com/ahobsonsayers/abs-goodreads/utils" + "github.com/samber/lo" ) const ( @@ -158,24 +158,33 @@ func (c *Client) GetBookByTitle(ctx context.Context, bookTitle string, bookAutho // SearchBooks search for a book by its title and optionally an author (which can give better results) // https://www.goodreads.com/api/index#search.books func (c *Client) SearchBooks(ctx context.Context, bookTitle string, bookAuthor *string) ([]Book, error) { - // Search for books via title, getting their ids. - bookIds, err := c.searchBookIdsByTitle(ctx, bookTitle) - if err != nil { - return nil, err - } + var bookIds []string + var err error + if bookAuthor == nil || *bookAuthor == "" { + // If author is not set, search for book ids by title + bookIds, err = c.searchBookIds(ctx, &bookTitle, nil) + if err != nil { + return nil, err + } - // If author is set, also search for books via author, getting their ids. - // Only keeps book ids that appear in both title and author searches - if bookAuthor != nil && *bookAuthor != "" { - authorBookIds, err := c.searchBookIdsByAuthor(ctx, *bookAuthor) + } else { + // If author is set, search for book id by title and author. + bookIds, err = c.searchBookIds(ctx, &bookTitle, bookAuthor) if err != nil { return nil, err } - // Get common book ids. If there are no common book - // ids, just use the book ids form the title search. - // Some result are better than none! - commonBookIds := utils.Intersection(bookIds, authorBookIds) + // Also search for book ids just by author + authorBookIds, err := c.searchBookIds(ctx, nil, bookAuthor) + if err != nil { + return nil, err + } + + // Get book ids common to both searches. + // This helps give better results that for the author. + // If there are no common book ids, just use the book ids + // from the first search. Some results are better than none! + commonBookIds := lo.Intersect(authorBookIds, bookIds) if len(commonBookIds) != 0 { bookIds = commonBookIds } @@ -190,45 +199,45 @@ func (c *Client) SearchBooks(ctx context.Context, bookTitle string, bookAuthor * return books, nil } -type bookIdUnmarshaller struct { - BookIds []string `xml:"search>results>work>best_book>id"` -} - -func (c *Client) searchBookIdsByTitle(ctx context.Context, title string) ([]string, error) { - queryParams := map[string]string{ - "q": title, - "search[field]": "title", - } - var unmarshaller bookIdUnmarshaller - err := c.Get(ctx, "search/index.xml", queryParams, &unmarshaller) - if err != nil { - return nil, err +func (c *Client) searchBookIds(ctx context.Context, title *string, author *string) ([]string, error) { + // Get query params + var query string + var searchField string + if title != nil && *title != "" && author != nil && *author != "" { + query = fmt.Sprintf("%s %s", *author, *title) + searchField = "all" + } else if title != nil && *title != "" { + query = *title + searchField = "title" + } else if author != nil && *author != "" { + query = *author + searchField = "author" + } else { + return nil, nil } - return unmarshaller.BookIds, nil -} - -func (c *Client) searchBookIdsByAuthor(ctx context.Context, author string) ([]string, error) { - var bookIds []string + bookIdsPages := make([][]string, 5) var errs error var wg sync.WaitGroup var bookIdsMutex sync.Mutex var errsMutex sync.Mutex - // Get 10 pages of author books. This should be enough - for pageNumber := 1; pageNumber <= 10; pageNumber++ { + // Get 5 pages of books. This should be enough + for idx := 0; idx < len(bookIdsPages); idx++ { wg.Add(1) - go func(page int) { + go func(idx int) { defer wg.Done() queryParams := map[string]string{ - "q": author, - "page": strconv.Itoa(page), - "search[field]": "author", + "q": query, + "search[field]": searchField, + "page": strconv.Itoa(idx + 1), + } + var unmarshaller struct { + BookIds []string `xml:"search>results>work>best_book>id"` } - var unmarshaller bookIdUnmarshaller err := c.Get(ctx, "search/index.xml", queryParams, &unmarshaller) if err != nil { errsMutex.Lock() @@ -238,11 +247,11 @@ func (c *Client) searchBookIdsByAuthor(ctx context.Context, author string) ([]st } bookIdsMutex.Lock() - bookIds = append(bookIds, unmarshaller.BookIds...) + bookIdsPages[idx] = unmarshaller.BookIds bookIdsMutex.Unlock() - }(pageNumber) + }(idx) } wg.Wait() - return bookIds, errs + return lo.Flatten(bookIdsPages), errs } diff --git a/goodreads/client_test.go b/goodreads/client_test.go index 585a214..2f293c5 100644 --- a/goodreads/client_test.go +++ b/goodreads/client_test.go @@ -6,7 +6,6 @@ import ( "testing" "github.com/ahobsonsayers/abs-goodreads/goodreads" - "github.com/ahobsonsayers/abs-goodreads/utils" "github.com/stretchr/testify/require" ) @@ -37,11 +36,7 @@ func TestGetBookByTitle(t *testing.T) { func TestSearch(t *testing.T) { client := goodreads.NewClient(http.DefaultClient, nil, nil) - books, err := client.SearchBooks( - context.Background(), - TheHobbitBookTitle, - utils.ToPointer(TheHobbitBookAuthor), - ) + books, err := client.SearchBooks(context.Background(), TheHobbitBookTitle, nil) require.NoError(t, err) checkTheHobbitBookDetails(t, books[0]) diff --git a/server/server.go b/server/server.go index 6747e7e..1d32317 100644 --- a/server/server.go +++ b/server/server.go @@ -4,7 +4,7 @@ import ( "context" "github.com/ahobsonsayers/abs-goodreads/goodreads" - "github.com/ahobsonsayers/abs-goodreads/utils" + "github.com/samber/lo" ) type server struct{} @@ -15,7 +15,7 @@ func (*server) Search(ctx context.Context, request SearchRequestObject) (SearchR // Search book goodreadsBooks, err := goodreads.DefaultClient.SearchBooks(ctx, request.Params.Query, request.Params.Author) if err != nil { - return Search500JSONResponse{Error: utils.ToPointer(err.Error())}, nil + return Search500JSONResponse{Error: lo.ToPtr(err.Error())}, nil } books := make([]BookMetadata, 0, len(goodreadsBooks)) diff --git a/utils/utils.go b/utils/utils.go deleted file mode 100644 index 420c25c..0000000 --- a/utils/utils.go +++ /dev/null @@ -1,28 +0,0 @@ -package utils - -import ( - "slices" - - mapset "github.com/deckarep/golang-set/v2" -) - -func ToPointer[T any](value T) *T { - return &value -} - -// Intersection returns the elements in slice 1 that also -// appear in slice 2. Returned slice order will match that -// of slice 1. If slice 1 has duplicates of a value that is -// present in slice 2, all duplicates will be included. -func Intersection[T comparable](slice1, slice2 []T) []T { - set2 := mapset.NewSet(slice2...) - - intersection := make([]T, 0, len(slice1)) - for _, elem := range slice1 { - if set2.Contains(elem) { - intersection = append(intersection, elem) - } - } - - return slices.Clip(intersection) -}