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

Image stack refactors #188

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

Conversation

fanckush
Copy link
Contributor

I would've liked to have the whole class refactored and then make a PR but I'm thinking if I make frequent PRs it will reduce the possibility of unnecessary merge conflicts.

I come from Python / JS , what I learned from Python is that things should be as clear as possible :).
This is my first attempt at refectory C++ so please throw all your constructive criticism at me now while I haven't formed my preferences and biased opinions on C++ conventions yet.

PR reason: #180

@fanckush
Copy link
Contributor Author

fanckush commented Sep 2, 2019

can anyone give this a look :)

@Beep6581
Copy link
Collaborator

Ping @Floessie @heckflosse

Copy link
Contributor

@Floessie Floessie left a comment

Choose a reason for hiding this comment

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

Well done! 👍

src/ImageStack.cpp Outdated Show resolved Hide resolved
src/ImageStack.cpp Outdated Show resolved Hide resolved
src/ImageStack.cpp Outdated Show resolved Hide resolved
src/ImageStack.cpp Outdated Show resolved Hide resolved
src/ImageStack.cpp Outdated Show resolved Hide resolved
src/ImageStack.cpp Outdated Show resolved Hide resolved
src/ImageStack.cpp Outdated Show resolved Hide resolved
src/ImageStack.cpp Outdated Show resolved Hide resolved
src/ImageStack.cpp Outdated Show resolved Hide resolved
src/ImageStack.cpp Show resolved Hide resolved
@fanckush fanckush mentioned this pull request Sep 15, 2019
@Beep6581 Beep6581 added this to the v0.7 milestone Sep 15, 2019
@ff2000
Copy link
Contributor

ff2000 commented Oct 7, 2019

I'm not bias against CamelCase or under_score notation. But IMO it is better to stick with what is already used, and as far as I can see that's CamelCase here in HdrMerge. Of course it's up to the owner/main devs to decide, but I have mixedFeelings with mixed_notation ;)
(As @Floessie already reviewed this PR and he didn't mention this it likely is OK.)

@Floessie
Copy link
Contributor

Floessie commented Oct 8, 2019

@ff2000 I didn't want to belabor mixed notation. Of course it would be nice to have a common style, but this shouldn't be strictly enforced, as this probably impedes contributors. It's up to @fanckush.

@fanckush
Copy link
Contributor Author

fanckush commented Oct 8, 2019

Good point, I personally still like this mixed notation because it adds an extra level of semantic separation between methods and variables which may reduce the ambiguity even a tiny bit.

This is just the beginning so it would be nice to decide now before i continue refactoring the rest. If everyone prefers camelCase that's fine by me too, maybe my python background is pulling me towards under_scores for variables but i'm used to both.

@fanckush
Copy link
Contributor Author

fanckush commented May 23, 2020

Please don't merge this PR just yet, some time has passed and i totally agree with what you are saying. will strictly use camelCase and drop snake_case because it's more consistent and introduces less changes to the code base 👍
Also in the chance that i don't refactor all the code, everything will be camelCase :)

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