Skip to content

Conversation

@HannaOdud
Copy link

Learners, PR Template

Self checklist

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

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.

@HannaOdud HannaOdud added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 1, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Apr 2, 2025

I shared with you some instructions to rebase your branch onto main in your Sprint-2 PR. If you have successfully rebased (and fixed) your Sprint-2 branch, can you also follow the same instructions to fix this branch?

@OracPrime OracPrime added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Apr 2, 2025
Copy link

@OracPrime OracPrime left a 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}`);

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 )

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

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.

// Three functions calls in lines 4,5,10
// 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

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


What does `console` store?
What does `console` store?
// Console store object;

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?

// Console store object;
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

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 ) {

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("");

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

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) {

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(true);
}
);
expect(result).toEqual(false);

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?

@OracPrime OracPrime added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Apr 2, 2025
@HannaOdud HannaOdud requested a review from OracPrime April 3, 2025 14:59
Copy link

@OracPrime OracPrime left a 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?

@HannaOdud
Copy link
Author

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.

@HannaOdud HannaOdud added the Complete Volunteer to add when work is complete and all review comments have been addressed. label Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed. Reviewed Volunteer to add when completing a review with trainee action still to take.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants