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

JS-Nerdery-Challenges/Rafael-David #78

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

RafoDev
Copy link

@RafoDev RafoDev commented Nov 15, 2024

Solution of the challenge algorithms.


const units = [hours, minutes, remainingSeconds];

const formattedUnits = units.map((unit) => unit.toString().padStart(2, '0'));
Copy link

@marceloev123 marceloev123 Nov 15, 2024

Choose a reason for hiding this comment

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

Nice way yo apply the needed unit transformation, I would prefer a better variable name like timeUnits instead of units but good work 👍


const formattedUnits = units.map((unit) => unit.toString().padStart(2, '0'));

return formattedUnits.join(':');

Choose a reason for hiding this comment

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

you can also return this directly right? but I understand why you separate this to have more readability 💯

return timeUnits.map((unit) => unit.toString().padStart(2, '0')).join(':');

@@ -41,7 +49,16 @@ Invoking "circularArray(2)" should return "["Island", "Japan", "Israel", "German
const COUNTRY_NAMES = ["Germany", "Norway", "Island", "Japan", "Israel"];

const circularArray = (index) => {
// YOUR CODE HERE...

Choose a reason for hiding this comment

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

What could happen if I send a negative number as an index here?

Choose a reason for hiding this comment

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

Which value will be appended if index is negative?

Copy link
Author

Choose a reason for hiding this comment

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

It will append an undefined at the beginning, I forgot to make that check.

.reduce((acc, currNum) => (acc + currNum) % modBase, 0)
.toString();

const boundedSumOfPowers = sumOfPowers.slice(sumOfPowers.length - lastDigits);
Copy link

@marceloev123 marceloev123 Nov 15, 2024

Choose a reason for hiding this comment

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

You can use a negative value in the slice function and it would have the same result

Suggested change
const boundedSumOfPowers = sumOfPowers.slice(sumOfPowers.length - lastDigits);
const boundedSumOfPowers = sumOfPowers.slice(-lastDigits);

@@ -70,7 +87,28 @@ The last 3 digits for the sum of powers from 1 to 10 is "317"
***** */

const ownPower = (number, lastDigits) => {
// YOUR CODE HERE...
const modBase = Math.pow(10, lastDigits + 1);

Choose a reason for hiding this comment

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

I like this approach to not overflow numbers and be precise in the calculations, you can also use a BigInt instead of it but good usage


const boundedSumOfPowers = sumOfPowers.slice(sumOfPowers.length - lastDigits);

return boundedSumOfPowers;

Choose a reason for hiding this comment

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

Good solution, I like the different approach maybe for future references bear in mind that some of the functions can be chained

const modBase = Math.pow(10, lastDigits + 1);
const sumOfPowers = Array.from({ length: number }, (_, idx) => {
    let poweredNumber = 1;
    for (let i = 0; i < idx + 1; i++) {
      poweredNumber = (poweredNumber * (idx + 1)) % modBase;
    }
    return poweredNumber;
  })
    .reduce((acc, currentNumber) => (acc + currentNumber) % modBase, 0)
    .toString();
    
   return sumOfPowers.slice(-lastDigits);

Copy link
Author

Choose a reason for hiding this comment

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

At first, I thought about sending it like that, but I wasn't sure how readable it would be. I'll keep it in mind for next time.


const serie = Array.from({ length: number }, (_, idx) => idx + 1);

const poweredSerie = serie.map((num) => {
Copy link

@marceloev123 marceloev123 Nov 15, 2024

Choose a reason for hiding this comment

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

I know why you declare this variable as num but maybe you can be more descriptive in your variables, don't be shy of using large naming, maybe serieNumber ?

});

const sumOfPowers = poweredSerie
.reduce((acc, currNum) => (acc + currNum) % modBase, 0)
Copy link

@marceloev123 marceloev123 Nov 15, 2024

Choose a reason for hiding this comment

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

same about naming conventions

Suggested change
.reduce((acc, currNum) => (acc + currNum) % modBase, 0)
.reduce((acc, currentNumber) => (acc + currentNumber) % modBase, 0)

factorialOfN *= i;
}

const digitsInfactorialOfN = [...factorialOfN.toString()].map(

Choose a reason for hiding this comment

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

Why spread operator is needed?

Copy link
Author

Choose a reason for hiding this comment

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

Because factorialOfN.toString() returns a string, using the spread operator splits the string into individual characters and place them in an array, allowing me to apply a map.


const sumOfDigits = digitsInfactorialOfN.reduce((acc, num) => acc + num, 0);

return sumOfDigits;

Choose a reason for hiding this comment

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

Clever solution you can apply the number cast directly to the reduce function instead of separate it in two steps though

const sumOfDigits = [...factorialOfN.toString()].reduce((acc, digit) => acc + Number(digit), 0);

let index = 2;

// f(n) = f(n-1) + f(n-2)
let n1 = 0;
Copy link

@marceloev123 marceloev123 Nov 15, 2024

Choose a reason for hiding this comment

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

Clear variables definition good work, however same thing about naming you can use other variable names

n1 = previousNumber
n2 = currentNumber

I would encourage you to use more descriptive names for variables

// f(n) = f(n-1) + f(n-2)
let n1 = 0;
let n2 = 1;
let fn = 1;

Choose a reason for hiding this comment

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

I think fn can be removed, you can use n2 variable and have the same result right?

Copy link
Author

Choose a reason for hiding this comment

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

That's true, I hadn't noticed it.

Copy link

@marceloev123 marceloev123 left a comment

Choose a reason for hiding this comment

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

Good work on finishing your JS challenge 🎉 , I noticed that sometimes you tend to separate things in extra steps, I think that you did it intentionally for code readability, however bear in mind that extra steps sometimes are unnecessary, I would like you to improve variables naming for future exercise too, good solutions so far, I think you have a good understanding when to use declarative functions

- Suggested more descriptive variable names for better clarity.
- Removed redundant lines by chaining functions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants