Skip to content

Implement sum #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Implement sum #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d743d53
initial commit
dorelljames Jan 30, 2023
fa4a286
Update sum.ts
webriq Jan 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions pages/api/sum.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import type { NextApiRequest, NextApiResponse } from "next";
import { sum } from "@/pages/utils/sum";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use alias for sum sum: calculateSum


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function should have return type e.g

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const { num1, num2 } = req.query;
const result = sum(num1, num2);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@arielpasilang arielpasilang Feb 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 });
  }


res.status(200).json({ total: result });
Comment on lines +5 to +8
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 });

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@arielpasilang arielpasilang Feb 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
8 changes: 8 additions & 0 deletions pages/utils/sum.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export function sum(a: number, b: number, format: "html" | "text" = "html") {
Copy link

@thisisdajaaa thisisdajaaa Feb 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Will do.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@dorelljames dorelljames Feb 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

@thisisdajaaa thisisdajaaa Feb 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function should have return type e.g

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@thisisdajaaa thisisdajaaa Feb 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I agree. Lets do that.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Copy link
Member Author

@dorelljames dorelljames Feb 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to expand @DAJAKMPM no need for else statement

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

}
}