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
Added Segmented Sieve of Eratosthenes [C++] #486
Conversation
Hi! Thanks for showing interest in contributing to this repository. Please make sure that you have ticked the points mentioned in PR description correctly. Thanks again! |
@singhpratyush continuous-integration/travis-ci/pr is showing build failed because of some indentation problem.. How can one resolve this? Please check and tell where did it go wrong. :) I don't think this is a problem. |
@manish1997 I checked your travis log, it seems that you have used spaces for indentation but we use tabs for C and CPP, to pass the tests you need to indent your whole code with tabs. |
@Monal5031 I edited the code to have only tabs. Still showing error.. Can you provide exact location to 2-3 points where this error is present? One example: I have used tab here. How to correct this. |
@Monal5031 @singhpratyush Finally continuous-integration/travis-ci/pr passed. Review the PR and merge it. :) |
@manish1997 Add an entry in README.md |
@@ -0,0 +1,57 @@ | |||
//Code Copyright: Manish Kumar, E&C, IIT Roorkee |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have copyright comments for this repo 😅
@aashutoshrathi how to add in readme. Instructions not given in contribution.md . |
@manish1997 Yes please remove the copyrights, and you can use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at the comments below. The points where I mentioned about spaces are not compulsory but would be good if you can make them as it would become consistent with other codes.
README.md
Outdated
@@ -46,6 +46,7 @@ Community (college) maintained list of Algorithms and Data Structures implementa | |||
| [Rod Cutting Problem](http://www.geeksforgeeks.org/dynamic-programming-set-13-cutting-a-rod/) | [:white_check_mark:](rod_cutting_problem/rod_cutting.c) | | [:white_check_mark:](rod_cutting_problem/RodCutting.java) | [:white_check_mark:](rod_cutting_problem/rod_cutting.py) | [:white_check_mark:](rod_cutting_problem/rod_cutting.go) | [:white_check_mark:](rod_cutting_problem/rodCuttingProblem.js) | | | |||
| [Shell Sort](https://en.wikipedia.org/wiki/Shellsort) | | [:white_check_mark:](shell_sort/ShellSort.cpp) | [:white_check_mark:](shell_sort/ShellSort.java) | [:white_check_mark:](/shell_sort/shell_sort.py) | [:white_check_mark:](shell_sort/shell_sort.go) | [:white_check_mark:](shell_sort/shellSort.js) | [:white_check_mark:](shell_sort/ShellSort.cs) | | |||
| [Sieve of Eratosthenes](https://en.wikipedia.org/wiki/Sieve_of_Eratosthenes) | [:white_check_mark:](sieve_of_eratosthenes/sieveOfEratosthenes.c) | | [:white_check_mark:](sieve_of_eratosthenes/SieveOfEratosthenes.java) | [:white_check_mark:](sieve_of_eratosthenes/sieve_of_eratosthenes.py) | [:white_check_mark:](sieve_of_eratosthenes/sieve_of_eratosthenes.go) | [:white_check_mark:](sieve_of_eratosthenes/sieveOfEratosthenes.js) | | | |||
| [Segmented Sieve of Eratosthenes](https://en.wikipedia.org/wiki/Sieve_of_Eratosthenes) | |[:white_check_mark:](segmented_sieve_of_eratosthenes/segmentedSieveOfEratosthenes.cpp)| | | | | | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The algorithms are supposed to be in ascending order, so this should go up.
Or maybe you can put it as Sieve of Eratosthenes (Segmented)
so that these two appear together and one can compare them.
#include <vector> | ||
using namespace std; | ||
|
||
vector<int> Prime; //contains prime numbers uptop segMax |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use global variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so that one can directly use Prime vector to find get all prime numbers just after calling segmentedSieve()..
|
||
vector<int> Prime; //contains prime numbers uptop segMax | ||
void segmentedSieve(){ | ||
#define segMax 1000000 //till point you want to find Primes. Preferebly pow(10,x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better take this as a parameter to the function, instead of hardcoding it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure will change it
high += rootSegMax; | ||
} | ||
} | ||
int main(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add line break between methods.
high += rootSegMax; | ||
} | ||
} | ||
int main(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int main(){
-> int main() {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also make this change at other positions too, like with conditionals and loops. Have a space before {
.
segmentedSieve(); //run it to find primes.. | ||
//Now Prime contains all the prime numbers upto segMax defined in function | ||
for(int i = 0; i < Prime.size(); i++){ | ||
cout << Prime[i] <<" "; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add space around <<
.
// ^^ higher end of current block we are finding primes for | ||
int tempMax = Prime.size(); | ||
// ^^All non prime numbers will be multiples of these numbers only | ||
while(low < segMax){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep a space between (
and reserved keywords that use them.
while(
-> while (
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it should be necessary. :)
@singhpratyush i will do changes by sunday :) |
@manish1997 Please resolve conflicts and make the requested changes. |
@Monal5031 there were no conflicts when I made this PR first. Someone tried merging it and conflicts came. Please look into that. Will make the changes by evening. |
@Monal5031 @singhpratyush check this now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few changes, rest of the code looks good
@@ -46,8 +46,8 @@ Community (college) maintained list of Algorithms and Data Structures implementa | |||
| [Rod Cutting Problem](http://www.geeksforgeeks.org/dynamic-programming-set-13-cutting-a-rod/) | [:white_check_mark:](rod_cutting_problem/rod_cutting.c) | | [:white_check_mark:](rod_cutting_problem/RodCutting.java) | [:white_check_mark:](rod_cutting_problem/rod_cutting.py) | [:white_check_mark:](rod_cutting_problem/rod_cutting.go) | [:white_check_mark:](rod_cutting_problem/rodCuttingProblem.js) | | | |||
| [Shell Sort](https://en.wikipedia.org/wiki/Shellsort) | | [:white_check_mark:](shell_sort/ShellSort.cpp) | [:white_check_mark:](shell_sort/ShellSort.java) | [:white_check_mark:](/shell_sort/shell_sort.py) | [:white_check_mark:](shell_sort/shell_sort.go) | [:white_check_mark:](shell_sort/shellSort.js) | [:white_check_mark:](shell_sort/ShellSort.cs) | | |||
| [Sieve of Eratosthenes](https://en.wikipedia.org/wiki/Sieve_of_Eratosthenes) | [:white_check_mark:](sieve_of_eratosthenes/sieveOfEratosthenes.c) | | [:white_check_mark:](sieve_of_eratosthenes/SieveOfEratosthenes.java) | [:white_check_mark:](sieve_of_eratosthenes/sieve_of_eratosthenes.py) | [:white_check_mark:](sieve_of_eratosthenes/sieve_of_eratosthenes.go) | [:white_check_mark:](sieve_of_eratosthenes/sieveOfEratosthenes.js) | | | |||
| [Sleep Sort](http://www.geeksforgeeks.org/sleep-sort-king-laziness-sorting-sleeping/) | | [:white_check_mark:](sleep_sort/sleep_sort.cpp) | [:white_check_mark:](sleep_sort/SleepSort.java) | [:white_check_mark:](sleep_sort/sleep_sort.py) | [:white_check_mark:](sleep_sort/sleep_sort.go) | | | | | |||
|
|||
| [Sieve of Eratosthenes (Segmented)](https://en.wikipedia.org/wiki/Sieve_of_Eratosthenes) | |[:white_check_mark:](segmented_sieve_of_eratosthenes/segmentedSieveOfEratosthenes.cpp)| | | | | | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the file name also be segmented_sieve_of_eratosthenes
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Monal5031 no. for c & c++ it should be like this.
vector<int> segmentedSieve(int segMax) { | ||
vector<int> Prime; //contains prime numbers uptop segMax | ||
//segMax is point till you want to find Primes. ALWAYS pow(10,x) | ||
int rootSegMax = sqrt(segMax); //root of segMax |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow standard C++ guidelines to name variables and functions throughout the code, use underscores to separate them rather than using camelCase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Monal5031 is it really necessary? I don't think it will add any value.. if u think otherwise let me know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manish1997 Sorry but we follow all the coding guidelines in this repository but we ignore many of them in ChefLib
. This repo was made for learning purpose and well documented code is the best way to learn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Monal5031 okay, I understand. but I am habitual of this name convention and I don't think changing these will help me learn anything or it will help anyone else in learning it a better way. No offense :) Please close this PR & #494 :)
#include <vector> | ||
using namespace std; | ||
|
||
vector<int> segmentedSieve(int segMax) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document the function, see https://github.com/iiitv/algos/blob/master/binary_search/binary_search.c for reference.
@manish1997: Please make the requested changes. Thanks. |
Closing this due to inactivity. |
https://en.wikipedia.org/wiki/Sieve_of_Eratosthenes#Segmented_sieve
Fixes #485
By submitting this pull request I confirm I've read and complied with the below declarations.
Added {Algorithm/DS name} [{Language}]
, notUpdate README.md
orAdded new code
.