Skip to content

Conversation

@bhawna94
Copy link

@bhawna94 bhawna94 commented Jul 20, 2020

Simplify expressions to enhance code readability.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 89.72% when pulling 29965a7 on bhawna94:simplify-expression into 28a1de4 on apache:master.

@namannigam-zz
Copy link

namannigam-zz commented Jul 20, 2020

Looks more like formatting changes. If not, can you revert all such formatting differences to specifically point out which expression is simplified?

@sebbASF
Copy link
Contributor

sebbASF commented Aug 5, 2020

Whilst the operations with 0 don't actually do anything, they help to make the algorithm clearer, so I don't think the code should be changed. Or, if the code is changed, there needs to be documentation to note that the changed code is part of the same sequence.
In any case, won't the compiler optimise the operations away?

@garydgregory
Copy link
Member

-1 Same issue as @sebbASF mentions: the code follows a pattern for obvious clarity.

@jochenw
Copy link
Contributor

jochenw commented Nov 14, 2020

I would support a change, that splits the code into multiple statements. To give an idea of what I am thinking:

    byte byte0 = /* Whatever's necessary to calculate one byte */;
    byte byte1 = /* Calculate another byte */;
    byte byte2 = /* Calculate the third byte */
    byte byte3 = /* Calculate the lasr´t byte */
    long result = byte0 | (byte1 << 8) | (byte2 << 16) | (byte3 << 24);

In my opinion, that would be actually readable. The current code as well as this proposed change aren't.

@garydgregory
Copy link
Member

Unless someone update the PR, we should close it IMO.

@garydgregory
Copy link
Member

No feedback, closing.

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.

6 participants