Skip to content

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

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

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 Participant to add when requesting review label Apr 1, 2025
@cjyuan
Copy link

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?

@cjyuan cjyuan added the 👀 Review Git Changes requested to do with Git label Apr 2, 2025
@OracPrime OracPrime added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Participant to add when requesting review 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 )

@@ -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

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

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;

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

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(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 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 Participant to add when work is complete and 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 Participant to add when work is complete and review comments have been addressed 👀 Review Git Changes requested to do with Git Reviewed Volunteer to add when completing a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants