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

Crisp branch #8

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Crisp branch #8

wants to merge 7 commits into from

Conversation

CrislpWW2
Copy link

No description provided.

@CrislpWW2 CrislpWW2 marked this pull request as ready for review June 14, 2023 14:05
@CrislpWW2 CrislpWW2 closed this Jun 14, 2023
@CrislpWW2 CrislpWW2 reopened this Jun 14, 2023
@CrislpWW2 CrislpWW2 requested a review from songrenzhao June 14, 2023 14:10
Copy link

@songrenzhao songrenzhao left a comment

Choose a reason for hiding this comment

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

Congrats on your first PR.
A few things that you should consider

  • You are writing a lot of imperative code, can we be a bit more functional?
    • Look into usage of Array.prototype.forEach(), Array.prototype.find(), Array.prototype.map()
  • You are nesting too much logic in a single function, with numerous if and else, can we make the function cleaner and easier to read?

Consider doing something below

  • Read each word
  • Check if current word is stopWords
  • Check if current word is mealTime
  • Check if current word is dates
  • Check if current word is unknown
const formattedWords = words.map(word => {
  const loweredCaseWord = word.toLowerCase();
  const isStopWord = stopWordList.includes(loweredCaseWord);
  if (isStopWord) {
     /// Your logic here ...
     return { ... }
  }

  const mealTime = mealTimes.find(...); // This will return mealTIme or undefined
  const datesLog = datesLogs.find(...); // This will return dateLog or undefined
  const mealTimeOrDatesLog = mealTime ?? datesLog;

  const response = !!mealTimeOrDatesLog ? { ... } : { ... };
  return response;
})

names: [
"tomorrow"
]
},
];

module.exports = {MealTimes, Dates};
module.exports = {mealTimes, datesLog};

Choose a reason for hiding this comment

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

  • It's important to be consistent
Suggested change
module.exports = {mealTimes, datesLog};
module.exports = { mealTimes, datesLog };

//TODO Fill this in
return false;
};
module.exports = { stopWordList }

Choose a reason for hiding this comment

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

Suggested change
module.exports = { stopWordList }
module.exports = { stopWordList };

Comment on lines +20 to +24
router.post('/parse', function (req, res) {
const { str } = req.body;
const parsed = parseFunction.parser(str);
res.json(parsed);
});

Choose a reason for hiding this comment

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

Maybe you can break these into controllers. So it's easier to maintain in the future

Suggested change
router.post('/parse', function (req, res) {
const { str } = req.body;
const parsed = parseFunction.parser(str);
res.json(parsed);
});
router.post('/parse', parse);

res.send("Parse It!");
});

//TODO Fulfill endpoint to list Meal Times

Choose a reason for hiding this comment

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

We can remove unnecessary comments

});

module.exports = router;
// end line

Choose a reason for hiding this comment

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

This is not needed, we don't need a comment to specify EOL. A new line is enough

Comment on lines +6 to +7
const regex = /[`~!@#$%^&*()_|+\-=?;:,'.<>\{\}\[\]\\\/]/gi; //special characters.
string = string.replace(regex , "").trim().split(" "); // filter out special characters and spaces.

Choose a reason for hiding this comment

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

  • This can be extracted into its middleware. We can do something like this
router.post('/parse', formatBody, parse);
  • Reason this is recommended is the concept of separation of concerns. We want to make the function as simple as possible, these types of validations can be handled before it reaches to the handler


class Parser {
//TODO Parse through and identify all StopWords & Identifiers
function parser(string) {

Choose a reason for hiding this comment

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

  • We should not name a function parameter string
  • string is usually a type in other languages, we should use words or other meaningful plurals

string = string.replace(regex , "").trim().split(" "); // filter out special characters and spaces.

for (let i = 0; i < string.length; i++) {
let lowerCaseWord = string[i][0].toLowerCase() + string[i].slice(1);

Choose a reason for hiding this comment

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

  • let is rarely recommended because we want to avoid mutability, if you believe lowerCaseWord won't be reassigned then we should use const
  • Can we just do string[i].toLowerCase()

Comment on lines +12 to +26
for (let j = 0; j < mealTimes.length; j++) { // nested loop to check if the word is in mealTimes array or datesLog array.
if (mealTimes[j].names.includes(lowerCaseWord)) {
objStringArray.push({
matchedWord: `${ string[i] }`,
type: `meal type`,
value: `${ mealTimes[j].value} `,
});
} else if ( datesLog[j] && datesLog[j].names.includes(lowerCaseWord)) {
objStringArray.push({
matchedWord: `${ string[i] }`,
type: `date`,
value: `${ datesLog[j].value }`,
});
}
}

Choose a reason for hiding this comment

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

type: `meal type`,
value: `${ mealTimes[j].value} `,
});
} else if ( datesLog[j] && datesLog[j].names.includes(lowerCaseWord)) {

Choose a reason for hiding this comment

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

  • mealTimes and datesLog don't share the same length
  • If j = mealTimes.length - 1, then datesLog[j] will be out of bound

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