Skip to content
This repository was archived by the owner on Oct 23, 2023. It is now read-only.

Completed core functionality (ZACHARY NORCROSS)#4

Open
znorcross211 wants to merge 3 commits intointern-practicefrom
intern-zacharyNorcross
Open

Completed core functionality (ZACHARY NORCROSS)#4
znorcross211 wants to merge 3 commits intointern-practicefrom
intern-zacharyNorcross

Conversation

@znorcross211
Copy link
Copy Markdown

No description provided.

Comment thread server/app.js
const app = express();

//TODO: Add an express router
const router = express.Router();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

Comment thread server/app.js Outdated
// Setup middleware so we can read the requests body
app.use(express.json());
app.use(express.urlencoded({ extended: true }));
app.use("/", router); // so router can work
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Move this router into a new file?

Comment thread server/app.js Outdated

// TASK 1
router.get("/mealtimes", (req, res) => {
if (req.query.query == undefined) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if (req.query.query) is better.

But Preferred:
const { query } = req.query;

Comment thread server/app.js Outdated
if (req.query.query == undefined) {
// if no query is given in the url
resultingJSON = { status: "success", data: [] };
resultingJSON.data = Dates;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Try to use immutability!

Comment thread server/app.js Outdated
if (req.query.query == undefined) {
// if no query is given in the url
resultingJSON = { status: "success", data: [] };
resultingJSON.data = stopWordList;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same thing as above.

Comment thread server/app.js Outdated
if (isStopWord(inputString)) {
// if we have a stop word
data.type = "stopword";
var word = getStopWord(inputString);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Try not to use var -> use const

Comment thread server/app.js Outdated

function isDateWord(inputString) {
for (var i = 0; i < Dates.length; i++) {
if (Dates[i].names.includes(inputString.toLowerCase())) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a way to simplify this?

Comment thread server/models/StopWord.js Outdated
const isStopWord = (word) => {
const isStopWord = word => {
//TODO Fill this in
if (stopWordList.includes(word.toLowerCase())) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you simplify this?

@pcdroidski
Copy link
Copy Markdown
Contributor

Looks good!

  1. Your app.js file is a big too big. Can you refactor it?
  2. Look into using services and/or actions.
  3. Add some unit tests using Jest

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants