Merge pull request #3 from hhow09/feat/improvement

improve after review and suggestion
basically reafctor and rename

backend/README.md

@@ -48,6 +48,8 @@ src
     ├── chat-repo.ts
     └── index.ts
 ```
+- dependency injection is used to decouple components for (1) separation of concerns and (2) better testability.
+- hierarchy: app -> chat-server -> command-service -> chat-repo -> mongodb
 
 ## MongoDB Data Modeling
 - Based on the requirement of `history` command (the only read operation), the data needed to be persisted is `clientId`, `operation`, and `result`.
@@ -91,4 +93,4 @@ src
     - e.g. `5 * -3` will return error.
 - large number will be converted to exponential notation: e.g. `999999999999 * 999999999999` will return `9.99999999998e+23`
     - default threshold of exponent is `20`
-    - ref: https://mikemcl.github.io/decimal.js/#precision
+    - ref: https://mikemcl.github.io/decimal.js/#toExpPos

backend/src/app.spec.ts

@@ -9,7 +9,7 @@ describe("App Integration Test", () => {
     let chatServer: ChatServer;
     let mongoClient: MongoClient;
     let clientSocket: ClientSocket;
-    let close: () => void;
+    let close: () => Promise<void>;
     const mongoUri = process.env.MONGODB_URI || "mongodb://localhost:27017";
     const serverPort = 3001;
     const serverUri = `http://localhost:${serverPort}`;

backend/src/app.ts

@@ -19,17 +19,21 @@ function connectMongo(uri: string): Promise<MongoClient> {
 }
 
 async function main() {
-    const logger = pino();
+    // setup connection
+    // fail first if connection fail
     const mongoClient = await connectMongo(uri);
+
+    // construct component
+    const logger = pino();
     const collection = await getMongoCollection(mongoClient, dbName, collectionName);
     const commandService = new CommandService(logger, new ChatRepo(collection));
     const chatServer = new ChatServer(logger, commandService, 3000);
-    const serverClose = await chatServer.listen();
+    const serverClose = chatServer.listen();
 
     // graceful shutdown
-    const shutdown = () => {
-        serverClose();
-        mongoClient.close();
+    const shutdown = async () => {
+        await serverClose();
+        await mongoClient.close();
     }
     process.on('SIGTERM', shutdown);
     process.on('SIGINT', shutdown);

backend/src/chat-server.ts

@@ -42,6 +42,8 @@ export class ChatServer {
                 try {
                     const history = await this.commandService.getHistory(socket.id);
                     sessionLogger.info(`Returning history: ${JSON.stringify(history)}`);
+                    // There is no need to run JSON.stringify() on objects as it (socket.io) will be done for you.
+                    // ref: https://socket.io/docs/v4/emitting-events/#basic-emit
                     socket.emit('history', history);
                 } catch (error) {
                     this.handleSocketError(socket, sessionLogger, "history", error as Error);

backend/src/command-service.ts

@@ -1,7 +1,7 @@
 import { Logger } from "pino";
 import { CommandAndResult } from "./entities/command-result.entity";
 import { IRepository } from "./repositories";
-import { isValidCommand, evaluate } from "./math/evaluate";
+import { evaluate } from "./math/evaluate";
 
 export interface ICommandService {
     evaluateAndSave(clientId: string, expression: string): Promise<string>;
@@ -30,9 +30,6 @@ class CommandService implements ICommandService {
 
     // evaluate evaluate a mathematical expression
     private evaluate(expression: string): string {
-        if (!isValidCommand(expression)) {
-            throw new Error('Invalid command');
-        }
         return evaluate(expression);
     }
 }

backend/src/math/evaluate.ts

@@ -1,33 +1,44 @@
 import { Summand } from './types';
 const operators = new Set(['+', '-', '*', '/']);
 
-// isValidCommand checks if the command is a allowed mathematical expression
-function isValidCommand(s: string): boolean {
-    s = s.replace(/ /g,''); // remove all spaces
-    if (s.length === 0) {
+// isValidOperation checks if the operation is a allowed mathematical expression
+function isValidOperation(operation: string): boolean {
+    const cleanedOperation = operation.replace(/ /g,''); // remove all spaces
+    if (cleanedOperation.length === 0) {
         return false;
     }
-    // allowed characters: 0-9, +, -, *, /, .
-    const regex = /^[\d+\-*/.]+$/;
-    if (!regex.test(s)) {
+    if (!allowedChars(cleanedOperation)) {
         return false;
     }
-    // operators cannot be adjacent to each other
-    for (let i = 0; i < s.length - 1; i++) {
-        if (operators.has(s[i]) && operators.has(s[i + 1])) {
-            return false;
-        }
+    if (!noAdjacentOperators(cleanedOperation)) {
+        return false;
     }
-    // operators cannot be at the end of the string
-    if (operators.has(s[s.length - 1])) {
+    if (noOperatorsAtTheEnd(cleanedOperation)) {
         return false;
     }
     return true;
 }
 
+function allowedChars(operation: string): boolean {
+    return /^[\d+\-*/.]+$/.test(operation);
+}
+
+function noAdjacentOperators(operation: string): boolean {
+    for (let i = 0; i < operation.length - 1; i++) {
+        if (operators.has(operation[i]) && operators.has(operation[i + 1])) {
+            return false;
+        }
+    }
+    return true;
+}
+
+function noOperatorsAtTheEnd(operation: string): boolean {
+    return operators.has(operation[operation.length - 1]);
+}
+
 // evaluate evaluate a mathematical expression
 function evaluate(s: string): string {
-    if (!isValidCommand(s)) {
+    if (!isValidOperation(s)) {
         throw new Error('Invalid command');
     }
     s = s.replace(/ /g,''); // remove all spaces
@@ -72,4 +83,4 @@ const parseExpression = (s: string): Summand[] => {
     return summands;
 }
 
-export { isValidCommand, evaluate };
+export { evaluate };

backend/src/math/types.ts

@@ -79,7 +79,7 @@ export class Fraction {
     }
 
     public add(f: Fraction): Fraction {
-        const commonDenominator = getLcm(this.denominator, f.denominator);
+        const commonDenominator = getLowestCommonMultiple(this.denominator, f.denominator);
         const multiplyThis = commonDenominator.div(this.denominator);
         const multiplyF = commonDenominator.div(f.denominator);
         this.numerator = this.numerator.mul(multiplyThis);
@@ -95,16 +95,16 @@ export class Fraction {
     }
 }
 
-function getLcm(a: Decimal, b: Decimal): Decimal {
-    return a.mul(b).div(getGcd(a, b));
+function getLowestCommonMultiple(a: Decimal, b: Decimal): Decimal {
+    return a.mul(b).div(getGreatestCommonDivisor(a, b));
 }
 
-function getGcd(a: Decimal, b: Decimal): Decimal {
+function getGreatestCommonDivisor(a: Decimal, b: Decimal): Decimal {
     const max = Decimal.max(a, b);
     const min = Decimal.min(a, b);
     if (max.mod(min).eq(0)) {
         return min;
     } else {
-        return getGcd(max.mod(min), min);
+        return getGreatestCommonDivisor(max.mod(min), min);
     }
 }