-
-
Notifications
You must be signed in to change notification settings - Fork 117
ITP_GLASGOW_MAR | HANNA_MYKYTIUK | MODULE_STRUCTURING_AND_TESTING_DATA | SPRINT_1 #433
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
base: main
Are you sure you want to change the base?
Conversation
I shared with you some instructions to rebase your branch onto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some good code here, but also a number of errors and omissions. I think it is worth you working through the comments and correcting the highlighted errors and re-submitting.
const lastIndexOfDot = filePath.lastIndexOf("."); | ||
const ext = filePath.slice(lastIndexOfDot); | ||
console.log(`**** The ext pert of ${filePath} is ${ext}`); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the answer this exercise is looking for as it teaches you basic string manipulation, but if you are doing this for real, use a library (you'll get to those shortly - when you have, google path.parse )
Sprint-1/1-key-exercises/4-random.js
Outdated
@@ -2,6 +2,10 @@ const minimum = 1; | |||
const maximum = 100; | |||
|
|||
const num = Math.floor(Math.random() * (maximum - minimum + 1)) + minimum; | |||
console.log(num); | |||
// this variable "num" has random | |||
// generated value in range from 1 to 101 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should try running this code many times and look at the values of num (you might want a for loop to make it easier)? In particular can you in your for loop log the maximum value of num that is seen? Alternatively read the documents on Math.random more carefully.
// b) Run the code and identify the line where the error is coming from - why is this error occurring? How can you fix this problem? | ||
|
||
// The error in line 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've identified the error, but haven't made a fix
|
||
Answer the following questions: | ||
|
||
What does `console` store? | ||
What does `console` store? | ||
// Console store object; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand further? What does this actually mean, or provide for you?
What does the syntax `console.log` or `console.assert` mean? In particular, what does the `.` mean? | ||
//'.' - allows us to use functions of console object. | ||
// `console.log` and `console.assert` are calling of these functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No quite. console.log is the definition of the log function. console.log() is calling it. A subtle but important distinction which you'll make use of later
function repeat() { | ||
return "hellohellohello"; | ||
function repeat(str, num) { | ||
if (num == 0 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to check the documentation for str.repeat and consider whether all of your code is necessary
const str = "hello"; | ||
const count = -1; | ||
const repeatedStr = repeat(str, count); | ||
expect(repeatedStr).toEqual(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is expecting a "" result. However the requirement is to throw an error.
Have you run this test??
Also you should give the tests sensible descriptions to help readers understand what is going on
return false; | ||
} else { | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't right. You haven't got the syntax for testing regular expressions correct, so each of you search tests is returning undefined and undefined<0 is returning false, so it thinks almost anything is a valid password.
And you're also [attempting to] use case-insensitive searching. Is this what you want?
Try googling use of regex.test and see if it helps. Also be careful about hyphens in regular expressions
//const isValidPassword = require("./password-validator"); | ||
|
||
|
||
function passwordValidator(password) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No! You should be using require to get the existing function in from password-validator.js. We want to separate our main code (that we want to include in our applications) from our test code (that we don't), but we definitely want to duplicate the code. Have a search for how require works in javascript
expect(result).toEqual(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And when you run it, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These three changes are all good - I assume you'll be doing more commits to address the other comments?
Yes. I will do each sprint. |
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.