Skip to content

Conversation

@AhmadHmedann
Copy link

Learners, PR Template

Self checklist

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

Changelist

I have answered all questions in the Sprint 2

Questions

Can you give me feedback on my answer in Sprint2/5-strech-extend folder

@AhmadHmedann AhmadHmedann added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 20, 2025
// =============> write your new code here
// =============> As I predict it will throw a SyntaxError: Identifier 'str' has already been declared
//because in JavaScript we can not redeclare the same variable in the same scope.
function Capitalise(str){

Choose a reason for hiding this comment

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

In Javascript, we follow the convention of camelCase for functions and variables

function calculateBMI(weight, height) {
// return the BMI of someone based off their weight and height
}
let BMI=weight/(height*height);

Choose a reason for hiding this comment

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

Does this need to be let here?

Choose a reason for hiding this comment

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

Nice! Even cleaner - removing the variable entirely if we just return it immediately

function ConvertToUpperSnakeCase(s1)
{
let result = "";
for(let i=0;i<s1.length;i++)

Choose a reason for hiding this comment

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

Is there a standard library function we could use here?

else
result+=s1[i];
}
let UPPER_SNAKE_CASE=result.toUpperCase();

Choose a reason for hiding this comment

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

We typically follow the convention of camelCase for Javascript variables

// You should call this function a number of times to check it works for different inputs


function toPound(penceString)

Choose a reason for hiding this comment

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

This is functionally looking good 🙂

I would suggest using VS Code's format document feature - this will ensure your Javascript code is following a standard format. This makes it easier for yourself, and others, to read the code in the future 🙂

https://code.visualstudio.com/docs/editing/codebasics#_formatting

Comment on lines 85 to 90
currentTime = TimeAs12hours("00:00");
targetTime = "12:00 am";
console.assert(
currentTime === targetTime,
`current time:${currentTime}, Target Time:${targetTime}`
);

Choose a reason for hiding this comment

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

I get the following assertion error on this test case:

Assertion failed: current output: 12:00 am, target output: 12:00 pm

Copy link
Author

Choose a reason for hiding this comment

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

Thank you very much for your feedback, it was so clear and helpful. For 5-stretch-extend/format-time The Task was: "Your task is to write tests for as many different groups of input data or edge cases as you can, and fix any bugs you find." I added two assertion and both were failed so I rewrite the code,

Choose a reason for hiding this comment

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

This is a really great submission! A lot of my comments were "nitpicks", but you've cleaned your code up really well

Will mark as complete

Copy link
Author

Choose a reason for hiding this comment

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

Thank you so much for your kind feedback! I really appreciate your time and comments — they helped me improve my code quality.

@CameronDowner CameronDowner added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 21, 2025
@AhmadHmedann AhmadHmedann added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Oct 24, 2025
@CameronDowner CameronDowner added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 24, 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants