Implement sum by dorelljames · Pull Request #2 · reactcebu/code-review-practice

dorelljames · 2023-02-18T05:55:04Z

...

As per Rabby's suggestion, I agree this makes more sense.

thisisdajaaa · 2023-02-18T07:36:10Z

pages/utils/sum.ts

+  if (format === "html") {
+    return `<span>${result}</span>`;
+  } else {
+    return result;


Would it be much more readable to implement this code block as shown below?

if(format === "text") return result:

return <span>${result}</span>

How about we remove the else statement in this case? I think the code you provided behaves the same way with my implementation but I agree that it makes it cleaner if we remove the else at this point.

thisisdajaaa · 2023-02-18T07:37:57Z

pages/utils/sum.ts

@@ -0,0 +1,8 @@
+export function sum(a: number, b: number, format: "html" | "text" = "html") {


Kindly create a typing that contains the format value as shown below

type Format = "html" | "text"

And use the format type in the format parameter as a type

Good catch! Will do.

thisisdajaaa · 2023-02-18T07:38:58Z

pages/utils/sum.ts

@@ -0,0 +1,8 @@
+export function sum(a: number, b: number, format: "html" | "text" = "html") {


Maybe it's best to add a comment for this function e.g jsdocs to have descriptions for the functionality of this function

Yes. lets do that. Can you give me reference to JSDocs and how we can do that? Maybe you can also provide an example or reference link for me to check?

thisisdajaaa · 2023-02-18T07:39:55Z

pages/utils/sum.ts

@@ -0,0 +1,8 @@
+export function sum(a: number, b: number, format: "html" | "text" = "html") {


Maybe it's best to rename the function to "getSum" or "calculateSum" in order to know that this function calculates the sum

OK. I think this aligns with our coding standards / style guide.

thisisdajaaa · 2023-02-18T07:42:11Z

pages/utils/sum.ts

@@ -0,0 +1,8 @@
+export function sum(a: number, b: number, format: "html" | "text" = "html") {


Maybe it's better to rename parameter to "num1" or "num2" to make it more readable compared to a and b. What do you think?

Yes. I agree. Lets do that.

pages/utils/sum.ts

harlekoy · 2023-02-18T07:44:35Z

pages/api/sum.ts

+  const { num1, num2 } = req.query;
+  const result = sum(num1, num2);
+
+  res.status(200).json({ total: result });


Hey @dorelljames. Although my understanding of Typescript may not be very strong, I have made an effort to improve its readability of this. I would greatly appreciate it if you could update it to this version.

const total = sum(num1, num2); res.status(200).json({ total });

Yes, I like your idea. Let's do that.

reidsolon · 2023-02-18T07:47:32Z

pages/api/sum.ts

+
+export default function handler(req: NextApiRequest, res: NextApiResponse) {
+  const { num1, num2 } = req.query;
+  const result = sum(num1, num2);


const sum = calculateSum(num1, num2) res.status(200).json({ sum })

Can you provide more context to this? or what do you really mean to do with this?

reidsolon · 2023-02-18T07:47:38Z

pages/api/sum.ts

@@ -0,0 +1,9 @@
+import type { NextApiRequest, NextApiResponse } from "next";
+import { sum } from "@/pages/utils/sum";


Use alias for sum sum: calculateSum

zao99 · 2023-02-18T08:00:39Z

Add decriprion and link issue pls.

glennmichael123 · 2023-02-18T07:40:34Z

pages/utils/sum.ts

@@ -0,0 +1,8 @@
+export function sum(a: number, b: number, format: "html" | "text" = "html") {


function should have return type e.g

export function sum(a: number, b: number, format: "html" | "text" = "html"): string | number {

I believe we only output string here so this makes the type incorrect. Don't you think?

glennmichael123 · 2023-02-18T07:41:33Z

pages/api/sum.ts

+import type { NextApiRequest, NextApiResponse } from "next";
+import { sum } from "@/pages/utils/sum";
+
+export default function handler(req: NextApiRequest, res: NextApiResponse) {


function should have return type e.g

export default function handler(req: NextApiRequest, res: NextApiResponse): void {

I appreciate the return type but I think this is not necessary.

glennmichael123 · 2023-02-18T07:42:24Z

pages/utils/sum.ts

+  if (format === "html") {
+    return `<span>${result}</span>`;
+  } else {
+    return result;


to expand @DAJAKMPM no need for else statement

arielpasilang · 2023-02-18T07:45:43Z

pages/api/sum.ts

+  const { num1, num2 } = req.query;
+  const result = sum(num1, num2);
+
+  res.status(200).json({ total: result });


It will be good if we also return a message and status code. Like:
res.status(200).json({ statusCode: 200, message: "Success", total: result });

so that when using this API from the front-end side, it's easy to trace the status code or return the message from the front-end side.

arielpasilang · 2023-02-18T07:50:30Z

pages/api/sum.ts

+
+export default function handler(req: NextApiRequest, res: NextApiResponse) {
+  const { num1, num2 } = req.query;
+  const result = sum(num1, num2);


Add try catch handler and handle statusCode.

try { const { num1, num2 } = req.query; const result = sum(num1, num2); res.status(200).json({ statusCode: 200, message: "Success", total: result }); } catch (err) { res.status(400).json({ statusCode:400, message: "Error when summing up", total: null }); }

dorelljames and others added 2 commits January 30, 2023 13:16

initial commit

d743d53

Update sum.ts

fa4a286

As per Rabby's suggestion, I agree this makes more sense.

dorelljames requested review from arielpasilang and markgaudiano February 18, 2023 05:56

dorelljames self-assigned this Feb 18, 2023

dorelljames added the enhancement New feature or request label Feb 18, 2023

thisisdajaaa reviewed Feb 18, 2023

View reviewed changes

pages/utils/sum.ts Show resolved Hide resolved

harlekoy reviewed Feb 18, 2023

View reviewed changes

reidsolon reviewed Feb 18, 2023

View reviewed changes

glennmichael123 reviewed Feb 18, 2023

View reviewed changes

arielpasilang reviewed Feb 18, 2023

View reviewed changes

		@@ -0,0 +1,8 @@
		export function sum(a: number, b: number, format: "html" \| "text" = "html") {

		@@ -0,0 +1,9 @@
		import type { NextApiRequest, NextApiResponse } from "next";
		import { sum } from "@/pages/utils/sum";

Conversation

dorelljames commented Feb 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dorelljames Feb 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thisisdajaaa Feb 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dorelljames Feb 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thisisdajaaa Feb 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thisisdajaaa Feb 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zao99 commented Feb 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielpasilang Feb 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielpasilang Feb 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dorelljames Feb 18, 2023 •

edited

Loading

thisisdajaaa Feb 18, 2023 •

edited

Loading

dorelljames Feb 18, 2023 •

edited

Loading

thisisdajaaa Feb 18, 2023 •

edited

Loading

thisisdajaaa Feb 18, 2023 •

edited

Loading

arielpasilang Feb 18, 2023 •

edited

Loading

arielpasilang Feb 18, 2023 •

edited

Loading