Code review & reflections #10

s373r · 2022-02-15T08:31:30Z

A draft PR to be able to add GitHub comments at the current code

Here we go 🚀

PS. NOT FOR MERGE

s373r

@ynhhoJ, I made a quick look, added some comments

s373r · 2022-02-15T08:31:56Z

examples/README.md

missed a blank line

s373r · 2022-02-15T08:36:37Z

examples/README.md

@@ -10,4 +11,4 @@ yarn ts-node ./examples/searchAuthors.ts [author name] [page number] [items limi

 ```powershell
 yarn ts-node ./examples/searchBooksBySeries.ts [book name] [page number] [items limit count]


To run examples, we need to install ts-node (a dev dependency), so will be great to have a note about that

For example, npm install --include dev (or yarn variant, ideally both)

Btw, what about using npm scripts (package.json scripts section) here instead of full commands?

Yeah, i just added scripts for paginated API and without pagination

s373r · 2022-02-15T08:38:49Z

examples/README.md

@@ -1,3 +1,4 @@
+<!--- -->
 ### Search book by name
 Commands:
 ```powershell


A general comment:

In common case, better to use ```sh for shell commands since we do not have here any PowerShell specific stuff

s373r · 2022-02-15T08:40:07Z

package.json

@@ -1,3 +1,4 @@
+


Missed a lock file*, please commit it

package-lock.json or yarn.lock

s373r · 2022-02-15T08:46:00Z

package.json

 {
  "name": "flibusta-api",
  "version": "0.2.0",
-  "main": "index.js",
+  "main": "index.js" ,


not have a main script here since we have not build artifacts (e.g. js bundle)

Removed main, maybe i will add it back later

s373r · 2022-02-15T11:35:50Z

src/index.ts

@@ -22,6 +23,7 @@ class FlibustaAPI {
    this.GetAuthors = new GetAuthors(axiosController);
  }

+  //
  getBooksByName(name: string, page = 0, limit = 50) {


I have an API design question (here and below)

Preamble:
With the current design, I as an API user, need to make other API calls if a page is not last.
I'd like to get items (books) with one call.

What about encapsulate that kind of logic into internals?

Pagination is a good thing as an option but should we use it as default? I see, it heavily depends on internal logic but we can make a more friendly interface

Good tip!
Add two different APIs, for paginated result and just raw items.

s373r · 2022-02-15T11:39:24Z

src/flibustaApiHelper.ts

    }

    // NOTE:       584 книг
    const [booksItemsCountAsString] = stringMatch;
    const booksCountOnlyNumbers = StringUtils.getNumbersFromString(booksItemsCountAsString);

-    return parseInt(booksCountOnlyNumbers, 10);
+    return parseInt(booksCountOnlyNumbers, 10); //


In other places, we use modular variant: Number.parseInt(), I guess here we have to do it as well (for consistency reasons)

Thx, added it!

s373r · 2022-02-15T11:40:13Z

src/flibustaApiHelper.ts

@@ -89,14 +93,14 @@ abstract class FlibustaAPIHelper {
    const stringMatch = StringUtils.getStringMatches(booksOrTranslationsAsString, regexRule);

    if (isNil(stringMatch)) {
-      return FlibustaAPIHelper.NIL_RESULT;
+      return FlibustaAPIHelper.NIL_RESULT; //
    }

    // NOTE:       584 книг


an unnecessary comment

s373r · 2022-02-15T11:52:21Z

src/flibustaApiHelper.ts

 import AxiosController from './axiosController';
 import { PagesInformation } from '../types/pagesInformation';
 import { HTMLElement, parse, Node } from 'node-html-parser';
 import { isEmpty, isNil, parseInt } from 'lodash';
 import Author from '../types/authors';
 import Book from '../types/book';
-import StringUtils from './utils/string';
+import StringUtils from './utils/string'; //

 abstract class FlibustaAPIHelper {
  private static NIL_RESULT = -1;


Looks like a pure C thing :)

Since we have a strong type-system in TS, I'd like to suggest use Nullable-like result type

Some reflections -- it is easy to forget to check result value with some magic constant but if it will be a nullable value we have to do that on a type basis

My personal opinion regarding interface design in general: if the type system is strongly typed out of the box we need to use it to protect API users

Another point, any checks with magic constants is a runtime thing but type checking is a compiling (read transpiling) time thing

Implemented nullable type instead of "magic constant", thx

s373r · 2022-02-15T12:08:38Z

src/flibustaApiHelper.ts

@@ -1,16 +1,18 @@
+//
 import AxiosController from './axiosController';
 import { PagesInformation } from '../types/pagesInformation';


A general optional comment regarding this module cohesion

I'd like to suggest taking a look at the page model conception (https://testcafe.io/documentation/402826/guides/concepts/page-model)

It helps reduce dependencies in code by moving page-related things into separate classes.
I see you use the raw HTML parsing approach -- maybe for the next projects, I propose to play around Testcase framework. It can run a browser in headless mode and interact with pages as a user would do

QA automation guys usually use this approach to test any kind of frontends (not only sites, even native desktop applications). Since here we have a classical scraping task, we can use it too

pvlrmnnk · 2022-02-18T22:59:15Z

Dude, where are your tests? Your code is weak against unpredictable layout changes.

ynhhoJ · 2022-02-19T07:19:24Z

@pvlrmnnk , i created an issue for this #4 , i will try to add ASAP

ynhhoJ · 2022-02-20T11:57:57Z

Just added tests for Flibusta API. Help is still wanted for 100% coverage or advices how tests should be written. I have small experience on work with it.

#4

pvlrmnnk · 2023-11-23T19:45:24Z

You've made great progress. Thank you for your work.

A dummy commit to activate GitHub code review

85e2309

s373r commented Feb 15, 2022

View reviewed changes

This was referenced Feb 16, 2022

npm package #13

Closed

page modules #14

Open

ynhhoJ closed this Feb 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code review & reflections #10

Code review & reflections #10

s373r commented Feb 15, 2022

s373r left a comment

s373r Feb 15, 2022

ynhhoJ Feb 15, 2022

s373r Feb 15, 2022

s373r Feb 15, 2022

ynhhoJ Feb 16, 2022

s373r Feb 15, 2022

ynhhoJ Feb 15, 2022

s373r Feb 15, 2022

ynhhoJ Feb 15, 2022

s373r Feb 15, 2022

ynhhoJ Feb 15, 2022

s373r Feb 15, 2022

ynhhoJ Feb 16, 2022

s373r Feb 15, 2022

ynhhoJ Feb 15, 2022

s373r Feb 15, 2022

ynhhoJ Feb 15, 2022

s373r Feb 15, 2022

ynhhoJ Feb 16, 2022

s373r Feb 15, 2022

pvlrmnnk commented Feb 18, 2022

ynhhoJ commented Feb 19, 2022

ynhhoJ commented Feb 20, 2022

pvlrmnnk commented Nov 23, 2023

		@@ -10,4 +11,4 @@ yarn ts-node ./examples/searchAuthors.ts [author name] [page number] [items limi

		```powershell
		yarn ts-node ./examples/searchBooksBySeries.ts [book name] [page number] [items limit count]

Code review & reflections #10

Code review & reflections #10

Conversation

s373r commented Feb 15, 2022

s373r left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvlrmnnk commented Feb 18, 2022

ynhhoJ commented Feb 19, 2022

ynhhoJ commented Feb 20, 2022

pvlrmnnk commented Nov 23, 2023