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

request: increase code readability #180

Open
fanckush opened this issue May 12, 2019 · 5 comments
Open

request: increase code readability #180

fanckush opened this issue May 12, 2019 · 5 comments

Comments

@fanckush
Copy link
Contributor

I'm really interested in this project and would love to contribute and start closing some of the issues. To be honest, the code isn't very understandable, many variables don't have proper naming and I think that makes it difficult for new people who didn't initially write the code. Take this function for example:

void Image::computeResponseFunction(const Image & r) {

uint16_t * usePixels = &data[-reldy*width - reldx];
const uint16_t * rUsePixels = &r.data[-relrdy*width - relrdx];
// some code...
double nv = rUsePixels[pos];
double v = usePixels[pos];

Maybe that's how C++ devs write their code, I'm not sure as I've never worked on a C++ project.
If what I'm saying makes sense and you agree with me, maybe we can do it bit by bit a function/method at a time by renaming variables and adding some more comments, specially for the mathematical components.

@Benitoite
Copy link
Contributor

In college I learned the type_nameInCamelCase convention in ANSI C class.
@heckflosse @Floessie etc are probably the C/C++ experts if it comes to drafting a CONVENTIONS.md.

@fanckush
Copy link
Contributor Author

Cool. from my point of view, any convention is fine really as long as it's verbose :)

@heckflosse
Copy link
Collaborator

Imho @Floessie is the expert for coding conventions.

@Floessie
Copy link
Contributor

I think we all agree that having strong conventions might hamper creativity and are hard to enforce on an existing project with many contributors. Still there should be some guidelines...

For RawTherapee, we have a cautiously moderated CONTRIBUTING.md with some rough guidelines not meant to be too interfering.

If what I'm saying makes sense and you agree with me, maybe we can do it bit by bit a function/method at a time by renaming variables and adding some more comments, specially for the mathematical components.

This sounds reasonable. IMHO comments should give an overview of the algorithm. If the identifiers are clearly describing the intent, single lines don't need to be commented.

When refactoring don't only rename identifiers but also think about improvements:

  • C++11 gives many opportunities to simplify code (e.g. default initializing with = {} or default returning with return {};). When using auto think a bit about the reviewers reading diffs and guessing types. It's a double-edged sword...
  • Help the compiler and reviewers by using const as default for almost anything (modern languages require mutable for variables because that makes more sense).
  • Reduce scopes. This also means moving private functions with little or no dependency to this to an anonymous namespace inside the implementation.
  • Fix bugs on the way.

Although I'm not an official contributor to HDRMerge I'm very interested in it from a user's perspective and am really looking forward to your enhancements. 👍

HTH,
Flössie

@fanckush
Copy link
Contributor Author

I made this PR #181

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

No branches or pull requests

4 participants