Skip to content
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.

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

CYF ITP South Africa - Cape Town | Dawud Vermeulen | Module-Structuring-and-Testing-Data | Week 3 #216

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

Conversation

Dave-Vermeulen
Copy link

Learners, PR Template

Self checklist

  • [✅] I have committed my files one by one, on purpose, and for a reason
  • [✅] I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • [✅] I have tested my changes
  • [✅] My changes follow the style guide
  • [✅] My changes meet the requirements of this task

Changelist

  • I followed the instructions in the readme file to approaching sprint 3. I did not follow a order for tackling the tasks and after completing the tasks inside the Implement dir i discovered another one inside revise. I made changes in the form of code and/or comments on each of the sprint 3 files (apart from the readme's). I committed these changes on a separate branch for sprint 3 and made several commits.

Questions

I have raised my questions in the workshop and on Slack

Copy link

@CE-0 CE-0 left a comment

Choose a reason for hiding this comment

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

Here is some feedback

@@ -0,0 +1,11 @@
module.exports = function getAngleType(angle) {
if (angle === 90) return 'Right angle';
if (angle < 90) return 'Acute angle';
Copy link

Choose a reason for hiding this comment

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

Bear in mind that the code is executed line-by-line, so the function will return 'acute angle' for anything less than or equal to 0. How would you remedy this?

@@ -0,0 +1,11 @@
module.exports = function getAngleType(angle) {
if (angle === 90) return 'Right angle';
Copy link

Choose a reason for hiding this comment

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

Not wrong, but is it really best practice to use only "if" statements in this instance?

module.exports = function getAngleType(angle) {
if (angle === 90) return 'Right angle';
if (angle < 90) return 'Acute angle';
if (angle > 90 && angle < 180) return 'Obtuse angle';
Copy link

Choose a reason for hiding this comment

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

While this is not wrong, note that code often doesn't need everything written in explicit detail and can interpret things based on implications, so it's best to avoid redundancy to keep the code as neat and succinct as possible. Do you see which boolean in the above line of code and one of the lines below may be redundant?

@@ -0,0 +1,16 @@
function repeat(str,count){
Copy link

Choose a reason for hiding this comment

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

Instructions stated that function should return a string. E.g. "robin" repeated 3 times should return "robinrobinrobin".

@@ -0,0 +1,6 @@
function cardValidator(cardNumber){
Copy link

Choose a reason for hiding this comment

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

Where are the contents of the function?

if (num <= 1) {
return false;
} // add this for only positive integers
for (let i = 2; i <= num/2; i++) { // change to a for loop. start at lowest prime 2, stop at num (but maybe makes more sense to be num/2).
Copy link

Choose a reason for hiding this comment

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

This is correct, but is "num/2" really the most efficient stopping point for the loop?

return false;
} // add this for only positive integers
for (let i = 2; i <= num/2; i++) { // change to a for loop. start at lowest prime 2, stop at num (but maybe makes more sense to be num/2).
if (num % i === 0 && i !== num) { // embed a if that checks for the match
Copy link

Choose a reason for hiding this comment

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

At what point in the loop is 'i' going to be equal to 'num'?

@@ -20,6 +20,6 @@ console.log(find("code your future", "z"));
// Pay particular attention to the following:

// a) How the index variable updates during the call to find
Copy link

Choose a reason for hiding this comment

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

You did not answer question 'a'

@CE-0 CE-0 added the Reviewed Volunteer to add when completing a review label Dec 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewed Volunteer to add when completing a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants