Skip to content

Is power of two #358

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

Merged
merged 11 commits into from
Sep 30, 2021
Merged

Is power of two #358

merged 11 commits into from
Sep 30, 2021

Conversation

i-redbyte
Copy link
Member

Checks if a number is a power of two

siriak
siriak previously approved these changes Sep 27, 2021
siriak
siriak previously approved these changes Sep 27, 2021
Copy link
Member

@siriak siriak left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@tjgurwara99 tjgurwara99 left a comment

Choose a reason for hiding this comment

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

Very good use of bitwise operations. I would recommend you try to make the function names as succinct as possible without losing the readability.

tjgurwara99
tjgurwara99 previously approved these changes Sep 27, 2021
Copy link
Member

@tjgurwara99 tjgurwara99 left a comment

Choose a reason for hiding this comment

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

LGTM 😄

@tjgurwara99
Copy link
Member

tjgurwara99 commented Sep 27, 2021

I've marked it as approved but I still think the log function should be moved to the parent package. Up to you @siriak 😄

siriak
siriak previously approved these changes Sep 27, 2021
Copy link
Member

@siriak siriak left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@tjgurwara99 tjgurwara99 left a comment

Choose a reason for hiding this comment

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

Small change 😅 everything else is perfect 😄

@raklaptudirm
Copy link
Member

Wouldn't it be better if we created a new sub-package under the math package, and put the files their? What are your opinions?

@i-redbyte
Copy link
Member Author

Wouldn't it be better if we created a new sub-package under the math package, and put the files their? What are your opinions?

New subpackage for one function? I think this is redundant

@tjgurwara99
Copy link
Member

Wouldn't it be better if we created a new sub-package under the math package, and put the files their? What are your opinions?

Could you elaborate on what you mean? If you are talking about moving just one function, then I think its not worth it - creates a lot of clutter. But if you mean to move all power of two functions in a sub package then I can see the benefits. Either way, could you elaborate? @raklaptudirm

@raklaptudirm
Copy link
Member

I was saying that instead of moving the log based algorithm to another file as you had suggested, we could create a new sub-package under math and put all the functions there.

math/
 └─ powersOfTwo/
     └─ checkPowerOfTwo.go
     └─ checkPowerOfTwo_test.go

@i-redbyte
Copy link
Member Author

I was saying that instead of moving the log based algorithm to another file as you had suggested, we could create a new sub-package under math and put all the functions there.

math/
 └─ powersOfTwo/
     └─ checkPowerOfTwo.go
     └─ checkPowerOfTwo_test.go

I am generally satisfied with both approaches, so that as most people decide, so I will do

siriak
siriak previously approved these changes Sep 28, 2021
Copy link
Member

@siriak siriak left a comment

Choose a reason for hiding this comment

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

I'm fine with either approach too :)

@tjgurwara99
Copy link
Member

Ah ok, I guess this is a question of opinion so majority would rule. But I would say that the binary operation functions are good inside the binary directory but the algorithm that uses log doesn't necessitate a sub package in my opinion.

I guess my point would be that it depends on whether there would be other algorithms that would be added to the directory - personally I don't think there are any algorithms other than these algorithms that would go inside this subpackage - which is not a problem but it creates another layer of subdirectory for learners/explorers to browse through. We should try to keep the package structure as close to flat as possible unless there is a specific reason for creating separate package.

So I guess my vote would be a no to creating a subdirectory/subpackage for it 😅. But if @siriak and @raklaptudirm disagree then go ahead with a sub package approach 😄

@i-redbyte
Copy link
Member Author

i-redbyte commented Sep 28, 2021

Ah ok, I guess this is a question of opinion so majority would rule. But I would say that the binary operation functions are good inside the binary directory but the algorithm that uses log doesn't necessitate a sub package in my opinion.

I guess my point would be that it depends on whether there would be other algorithms that would be added to the directory - personally I don't think there are any algorithms other than these algorithms that would go inside this subpackage - which is not a problem but it creates another layer of subdirectory for learners/explorers to browse through. We should try to keep the package structure as close to flat as possible unless there is a specific reason for creating separate package.

So I guess my vote would be a no to creating a subdirectory/subpackage for it 😅. But if @siriak and @raklaptudirm disagree then go ahead with a sub package approach 😄

Yes, it looks like the subpackage will be redundant

Copy link
Member

@tjgurwara99 tjgurwara99 left a comment

Choose a reason for hiding this comment

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

Could you move the test for the UseLog function to the parent math folder? do this dependent on whether other maintainers agree with my previous suggestion.

@i-redbyte
Copy link
Member Author

Could you move the test for the UseLog function to the parent math folder? do this dependent on whether other maintainers agree with my previous suggestion.

Done

@i-redbyte i-redbyte requested a review from siriak September 29, 2021 09:36
@i-redbyte i-redbyte changed the title Feat is power of two Is power of two Sep 29, 2021
@i-redbyte
Copy link
Member Author

So what do we decide in the end, gentlemen?

@siriak
Copy link
Member

siriak commented Sep 29, 2021

@raklaptudirm waiting on you :)

Copy link
Member

@raklaptudirm raklaptudirm left a comment

Choose a reason for hiding this comment

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

I was away for a bit, so could not reply earlier. All good. I am ok with the directory structure.

@siriak siriak merged commit 420ddd6 into TheAlgorithms:master Sep 30, 2021
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.

4 participants