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

adding get-simple-numbers.js #1

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

Conversation

burbokop
Copy link

No description provided.

Copy link
Member

@tshemsedinov tshemsedinov left a comment

Choose a reason for hiding this comment

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

Please follow same repository structure like in other HowProgrammingWorks repositories:

  • We place js examples in JavaScript, not in lib, because this is not library and we may implement this in other languages
  • Use eslint with .eslintrc.yml rules from other HPW repo, for example .eslintrc.yml but don't commit it in pull request with your code
  • Add 'use strict'; to your code
  • See other comment in code
  • Use eslint . before commit

@@ -0,0 +1,23 @@
sort = (array, divider) => {
Copy link
Member

Choose a reason for hiding this comment

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

Declare with const

}

console.log("array after erasing: " + array);
if(erased == 0) return array.sort((a,b) => a-b);
Copy link
Member

Choose a reason for hiding this comment

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

===

}

if(process.argv.length > 2) console.log("\nresult: " + getArrayOfSimpleNumbers(process.argv[2]));
else console.log("Program created for getting simple number using sieve of Eratosthenes.\nusage:\n node ./get-simple-numbers.js [renge of numbers]");
Copy link
Member

Choose a reason for hiding this comment

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

Max line length: 80 chars
Add empty line at the end of file

@@ -0,0 +1,23 @@
sort = (array, divider) => {
console.log("divider: " + divider);
Copy link
Member

Choose a reason for hiding this comment

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

Use single quotes

sort = (array, divider) => {
console.log("divider: " + divider);
let erased = 0;
for(value of array) if(value % divider === 0 && value != divider) {
Copy link
Member

Choose a reason for hiding this comment

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

Add spaces, code blocks and use !== instead of !=

@burbokop
Copy link
Author

burbokop commented Sep 22, 2018 via email

Copy link
Member

@tshemsedinov tshemsedinov left a comment

Choose a reason for hiding this comment

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

  • You have placed example to repository root folder, but we usually place into language-specific folders, JavaScript or Haskell

const sort = (array, divider) => {
console.log('divider: ' + divider);
let erased = 0;
for (const value of array) if (value % divider === 0 && value !== divider) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer:

for (...) {
  if (...) {
  }
}

return sort(array, array[0]);
};

if (process.argv.length > 2)
Copy link
Member

Choose a reason for hiding this comment

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

It would be better having operator block {} for then branch if we have block for else branch

if (process.argv.length > 2)
console.log('\nresult: ' + getArrayOfSimpleNumbers(process.argv[2]));
else {
console.log('Program allows getting simple number');
Copy link
Member

Choose a reason for hiding this comment

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

Program generates simple numbers using Sieve of Eratosthenes
  Usage: node ./generate-simple-numbers [max]
  Example: node ./generate-simple-numbers 100

@burbokop
Copy link
Author

burbokop commented Sep 23, 2018 via email

Copy link
Member

@tshemsedinov tshemsedinov left a comment

Choose a reason for hiding this comment

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

Now we can start with code refactoring:

  • Names getArrayOfSimpleNumbers and sort are not descriptive
  • Code decomposition to mentioned two functions in not optimal
  • Eratosthenes algorithm do not expect use of array.sort
  • I prefer loop, not recursion

@burbokop
Copy link
Author

burbokop commented Sep 24, 2018 via email

@nechaido
Copy link
Member

@tshemsedinov

Use eslint with .eslintrc.yml rules from other HPW repo, for example .eslintrc.yml but don't commit it in pull request with your code

We can put it in /JavaScript so that it will be used by linter and/or editor when running checks for that folder.

@burbokop
Copy link
Author

I've already done it

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.

3 participants