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.

Sign up for GitHub

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

PR-1 #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

PR-1 #1

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions calculate_average.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@

const calculateAverage = function calculateAverage(grades) {
Copy link

Choose a reason for hiding this comment

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

Here's a one-off single comment!


let total = 0;
for (let i = 0; i < grades.length; i++) {
Copy link

Choose a reason for hiding this comment

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

you could put grades.length into a variable to avoid recalculating it every loop

total += grades[i];
}

Copy link

Choose a reason for hiding this comment

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

This is really clean and readable. well sone

let avg = total / grades.length;
Copy link

Choose a reason for hiding this comment

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

does avg end up as an integer or a float? If all the grades are whole numbers, it might be an integer, but you probably want to force it to do float math

Copy link

Choose a reason for hiding this comment

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

avg could also be const instead of a let

Copy link

Choose a reason for hiding this comment

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

Consider renaming avg to avgGrades to be clearer.

Copy link

Choose a reason for hiding this comment

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

Suggested change
let avg = total / grades.length;
let avg = total / parseFloat(grades.length);


return avg;
Copy link

Choose a reason for hiding this comment

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

instead of storing the average as a variable, you could return the calculation return value directly

Suggested change
return avg;
return total / grades.length;

Copy link

Choose a reason for hiding this comment

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

I like what you've done here.

Copy link

Choose a reason for hiding this comment

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

Can delete line 9 and just do:

Suggested change
return avg;
return total / grades.length;

};

export default calculateAverage;
Copy link

Choose a reason for hiding this comment

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

Might want to add a newline here

Copy link

Choose a reason for hiding this comment

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

lgtm