refactoring/NR-65 Refactoring game identifier #207

lukatarman · 2023-12-18T15:50:16Z

Related Issue: #213

fixed missing argument in method bug

# removed unused code # fixed bugs in updateGamesWithoutDetails and updateGamesWithoutReleaseDates tests

# updated tests

# fixed small true/false logic bug # removed isGame method from steamApp # updated tests to reflect change

# renamed functions # fixed test descriptions # adjusted tests # slight refactor/final PR adjustments

lukatarman · 2024-02-06T09:20:12Z

backend/src/core/features/game-identifier/game.identifier.js

@@ -34,102 +34,63 @@ export class GameIdentifier {
    this.#options = options;
  }

-  tryViaSteamWeb = async () => {
-    this.#logger.debugc("identifying games via steam web");
+  tryIfGameViaSource = async (source) => {


Rename to checkIfGameViaSource

# moved steamAppsIsEmpty functionality to it # added instantiation to steamApps repo get method # added tests

# renamed aggregator # added tests

# passed in logger through the games repository into aggregator # removed fdescribe # added tests

# adjusted test to reflect code

# added tests

# improved test descriptions

# changed getter into normal method # adjusted tests, fixed some typos

# adjusted/fixed some tests

# improved test descriptions

stas-at-ibm

Great job man! You did a lot and a lot correct. I added some comments and ideas. You decide what to tackle or not. You can merge if you want.

stas-at-ibm · 2024-01-31T07:12:13Z

backend/src/core/models/game.js

@@ -11,6 +11,20 @@ export class Game {
  imageUrl;
  playerHistory;

+  copy() {


We have cloneDeep for that. It's used in the game class in line 93 also.

I tried using cloneDeep for the use cases in the app. Ran across a problem. From what I discovered, it seems like cloning private properties of an instance of a class is impossible through clone deep, and some suggestions said to just create your own method for doing this in the class. I'm not sure what to do at this point, but in 30 minutes of searching I couldn't find a better solution. Let me know if it's okay to just leave it how it is, or if I should search more/you have a different suggestion.

The error was: "TypeError: Receiver must be an instance of class SteamApp".

References:
lodash/lodash#5589
lodash/lodash#5247
https://stackoverflow.com/questions/57542052/deep-clone-class-instance-javascript

stas-at-ibm · 2024-01-31T07:12:56Z

backend/src/core/models/game.spec.js

 import { PlayerHistory } from "./player.history.js";
 import { getXSampleSteamApps } from "./steam.app.mocks.js";

 describe("game.js", function () {
  describe("Game", function () {
+    describe(".copy", function () {


remove once switched to deepCopy

stas-at-ibm · 2024-01-31T07:13:45Z

backend/src/core/models/steam.app.js

@@ -23,32 +21,20 @@ export class SteamApp {
    return copy;


when you're at it, use deepCopy here also

stas-at-ibm · 2024-01-31T07:17:36Z

backend/src/core/models/valid.data.sources.js

@@ -12,4 +12,13 @@ export class ValidDataSources {
    }
    return Object.freeze(enumObject);
  }
+
+  static getSourceUrl(id, source) {
+    if (source === ValidDataSources.validDataSources.steamWeb)


use switch/case

stas-at-ibm · 2024-04-02T15:33:08Z

backend/src/core/models/valid.data.sources.js

@@ -12,4 +12,13 @@ export class ValidDataSources {
    }
    return Object.freeze(enumObject);
  }
+
+  static getSourceUrl(id, source) {


Would leave in steam.client.js as a method. Its not the responsibility of the data model to know the exact URLs. Ideally we would have those URLs provide externally via Env Vars like all other external things (think DB connection url) so that we can set it to something else if we need to without changing the code. BUT we did not get to that yet, so I would just move it out here an into the steam client.

This will go away anyway because you will switch to the API, right?

stas-at-ibm · 2024-04-02T16:41:46Z

backend/src/core/models/steam.apps.aggregate.js

+export class SteamAppsAggregate {
+  apps;
+
+  static manyFromDbEntries(steamApps) {


use normal constructor

stas-at-ibm · 2024-04-02T16:41:53Z

backend/src/core/models/steam.apps.aggregate.js

+    return steamAppsAggregate;
+  }
+
+  isEmpty() {


stas-at-ibm · 2024-04-02T16:43:33Z

backend/src/core/models/steam.apps.aggregate.js

+    this.apps = this.apps.map((app) => {
+      const appCopy = app.copy();
+
+      const page = this.#findSteamAppHtmlDetailsPage(htmlDetailsPages, appCopy);


pass in app.appid

stas-at-ibm · 2024-04-02T16:44:46Z

backend/src/core/models/steam.apps.aggregate.spec.js

+  });
+
+  describe(".isEmpty", function () {
+    describe("when the steamApps array is empty", function () {


same as above...update text..in the tests below also

stas-at-ibm · 2024-04-02T16:51:21Z

backend/src/core/features/game-identifier/game.identifier.spec.js

-        this.steamAppsRepository = createSteamAppsRepositoryMock([], undefined);
-        this.gamesRepository = createGamesRepositoryMock();
-        this.historyChecksRepository = createHistoryChecksRepositoryMock();
+  describe(".checkIfGameViaSource.", function () {


You could heavily simplify the tests. No need to check for the calling or not calling of all methods of the mocks, it would be enough to check for the flow of the logic, e.g. when isEmpty is true => just check that nothing is persisted. And if isEmpty is false check only for the expected result to be persisted. Do you know what I mean?

Will skip this for now since API/logic will change heavily. Opened ticket to do it after: #212

# removed todo # fixed test using steamApps aggregate instead of games aggregate

# removed old test

# udpated tests

# encapsulated methods, made them private

# applicaiton now uses .content to access the content in games aggregate # modified tests to reflect new code

# renamed private mathod # changed argument of private method into game's id only

# removed unused methods

stas-at-ibm

Very good bro! Thanks for taking the time to add the changes.

See comments below. Only the copy one is a one I would recommend you add in your next branch if the code will be used further.

stas-at-ibm · 2024-04-12T12:46:05Z

backend/src/adapters/driven/http/steam.client.js


    try {
      return (await this.#httpClient.get(url)).data;
    } catch (err) {
      return "";
    }
  }
+
+  getSourceUrl(id, source) {


use private methods (encapsulation) if no client needs access

stas-at-ibm · 2024-04-12T13:03:56Z

backend/src/core/models/games.aggregate.js


-    return gamesAggregate;
+  get content() {
+    return this.#games;


Always return or copy, because JS uses "pass by reference" of objects (and arrays which is an object). If you don't use a copy the returned value can be modified and by that the GamesAggregate instance also and you did not intended that.

stas-at-ibm · 2024-04-12T13:04:29Z

backend/src/core/models/games.aggregate.js

-  isEmpty() {
-    if (this.games.length > 0) return false;
+  get isEmpty() {
+    if (this.#games.length > 0) return false;


return this.#games.length > 0

stas-at-ibm · 2024-04-12T13:05:10Z

backend/src/core/models/games.aggregate.js


      return gameCopy;
    });
  }

-  #findGamesHtmlDetailsPage(htmlDetailsPages, game) {
-    return htmlDetailsPages.find((page) => game.id === page.id).page;
+  findPageForGameById(htmlDetailsPages, gameId) {


stas-at-ibm · 2024-04-12T13:09:11Z

backend/src/core/models/steam.apps.aggregate.js


-    return steamAppsAggregate;
+  get content() {
+    return this.#apps;


return a copy

stas-at-ibm · 2024-04-12T13:10:50Z

backend/src/core/models/steam.apps.aggregate.spec.js

-        expect(this.steamAppsArray.apps[0].triedVia).toEqual(["steamWeb"]);
-        expect(this.steamAppsArray.apps[0].failedVia).toEqual(["steamWeb"]);
-        expect(this.steamAppsArray.apps[0].type).toBe("unknown");
+        expect(this.steamAppsArray.content[0].appid).toBe(1);


why not use just toEqual?

Tried using toEqual, but the object I compare it to has to be an instance of same class, seems like it would be unnecessarily extra work for no real benefit? Let me know if I should do it anyway, and I will do in the future.

lukatarman self-assigned this Dec 18, 2023

lukatarman linked an issue Dec 18, 2023 that may be closed by this pull request

Refactor game identifier, steam client and steam app data model #198

Closed

5 tasks

lukatarman force-pushed the refactoring/NR-65-refactoring-game-identifier branch from cb0a73e to c88d72a Compare January 16, 2024 15:09

lukatarman added 11 commits January 17, 2024 09:42

Halfway done with refactoring game identifier

f31dfa2

refactored get untried filtered steam apps method

098f3ea

added first part of tryIfGameViaSource test

2b85362

fixed test

8b05b8d

added second test case

3fe45fd

fixed missing argument in method bug

added all tryIfGameViaSource tests

1569b81

# removed unused code # fixed bugs in updateGamesWithoutDetails and updateGamesWithoutReleaseDates tests

added .copy() method to game class

7adf85e

added missing tests to game.service.js file

5fe657b

finished refactoring game identifier

378231d

refactored game identifier and game service slightly

8078f29

# updated tests

updated imports to reflect new folder structure

7a79a0d

# fixed small true/false logic bug # removed isGame method from steamApp # updated tests to reflect change

lukatarman force-pushed the refactoring/NR-65-refactoring-game-identifier branch from c88d72a to 7a79a0d Compare January 19, 2024 13:24

lukatarman added 3 commits January 19, 2024 16:18

removed unnecessary logger messages

cce9f5f

# renamed functions # fixed test descriptions # adjusted tests # slight refactor/final PR adjustments

added ValidDataSources test

bfe588d

fixed bug causing repeat adds of failed/tried via

8f37ea9

lukatarman commented Feb 6, 2024

View reviewed changes

lukatarman added 4 commits February 6, 2024 17:54

added todo

d33d876

created GamesAggregator

7f2fe0c

added logger to steamAppsRepository

71bd5d8

added steamAppsAggregate data model

ccbc45a

# moved steamAppsIsEmpty functionality to it # added instantiation to steamApps repo get method # added tests

lukatarman force-pushed the refactoring/NR-65-refactoring-game-identifier branch from 0fbe661 to ccbc45a Compare February 15, 2024 14:45

lukatarman added 6 commits February 16, 2024 15:10

added tests for steamAppsAggregate

730c290

added initial games.aggregate methods

58c83d2

# renamed aggregator # added tests

added todos

a4cdb19

added using GamesAggregate to games repository

15dbc1d

# passed in logger through the games repository into aggregator # removed fdescribe # added tests

Added message argument to games aggregate method

c8ce0a3

# adjusted test to reflect code

implemented new data model to game identifier

8d6fade

# added tests

lukatarman added 11 commits March 20, 2024 14:15

removed todo

3a334b9

fixed method duplication

b1a45e0

added todo

3647273

extracted reusable code into separate private method

973dc07

# improved test descriptions

optimized some tests

c928d55

reorganized method order

ecddf68

# changed getter into normal method # adjusted tests, fixed some typos

removed fdescribe

348a34f

removed unused method

7f29a48

# adjusted/fixed some tests

added values for readability

62a60d5

# improved test descriptions

fixed bug running collectSteamApps twice

1b1d2d2

fixed bug incorrectly persisting steamapps/games

2011408

stas-at-ibm approved these changes Apr 2, 2024

View reviewed changes

lukatarman added 14 commits April 7, 2024 11:26

changed isEmpty in games aggregate into getter

a867e63

# removed todo # fixed test using steamApps aggregate instead of games aggregate

moved getSourceUrl into steam client

722700e

# removed old test

changed getIds method to a getterr

e7bd160

renamed method to extractGameDetailsFrom

1ff3e59

made methods in fromSteamApp method private

e2d1af1

# udpated tests

renamed methods

15d0999

# encapsulated methods, made them private

changed games aggregate games property to private

900e877

# applicaiton now uses .content to access the content in games aggregate # modified tests to reflect new code

changed static method to standard constructor in games aggregate

c855f4a

changed games aggregate isEmpty method into getter

87f4166

# renamed private mathod # changed argument of private method into game's id only

changed SteamApp isGame method into getter

d316d19

made some methods in steam app private

fd2017d

# removed unused methods

made steam apps aggregate content private. Used content getter

b4256b0

changed steam apps aggregate static method into constructor

4b2ef79

passed only steam app id into private method in steam apps aggregate

024ae51

lukatarman removed a link to an issue Apr 11, 2024

Refactor game identifier, steam client and steam app data model #198

Closed

5 tasks

lukatarman linked an issue Apr 11, 2024 that may be closed by this pull request

Refactor game identifier #213

Closed

6 tasks

lukatarman merged commit d2e77db into master Apr 11, 2024
2 checks passed

stas-at-ibm reviewed Apr 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactoring/NR-65 Refactoring game identifier #207

refactoring/NR-65 Refactoring game identifier #207

lukatarman commented Dec 18, 2023 •

edited

Loading

lukatarman Feb 6, 2024

stas-at-ibm left a comment

stas-at-ibm Jan 31, 2024

lukatarman Apr 7, 2024

stas-at-ibm Jan 31, 2024

stas-at-ibm Jan 31, 2024

stas-at-ibm Jan 31, 2024

stas-at-ibm Apr 2, 2024

stas-at-ibm Apr 2, 2024

stas-at-ibm Apr 2, 2024

stas-at-ibm Apr 2, 2024

stas-at-ibm Apr 2, 2024

stas-at-ibm Apr 2, 2024

lukatarman Apr 10, 2024 •

edited

Loading

stas-at-ibm left a comment

stas-at-ibm Apr 12, 2024

stas-at-ibm Apr 12, 2024

stas-at-ibm Apr 12, 2024

stas-at-ibm Apr 12, 2024

stas-at-ibm Apr 12, 2024

stas-at-ibm Apr 12, 2024

lukatarman Apr 26, 2024

refactoring/NR-65 Refactoring game identifier #207

refactoring/NR-65 Refactoring game identifier #207

Conversation

lukatarman commented Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

stas-at-ibm 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

lukatarman Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

stas-at-ibm 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

lukatarman commented Dec 18, 2023 •

edited

Loading

lukatarman Apr 10, 2024 •

edited

Loading