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

favouring terciary operator over if...return...else...return #240

Merged
merged 1 commit into from
Feb 29, 2020

Conversation

Janther
Copy link
Contributor

@Janther Janther commented Feb 28, 2020

Not really necessary. It just feels nicer and a bit cleaner.

@Janther Janther added the refactor reorganising the code without changes label Feb 28, 2020
@codecov
Copy link

codecov bot commented Feb 28, 2020

Codecov Report

Merging #240 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #240      +/-   ##
==========================================
- Coverage    99.7%   99.68%   -0.03%     
==========================================
  Files          84       84              
  Lines         677      633      -44     
  Branches      115      113       -2     
==========================================
- Hits          675      631      -44     
  Misses          2        2
Impacted Files Coverage Δ
src/nodes/NumberLiteral.js 100% <100%> (ø) ⬆️
src/nodes/ForStatement.js 100% <100%> (ø) ⬆️
src/nodes/UnaryOperation.js 100% <100%> (ø) ⬆️
src/nodes/FunctionDefinition.js 100% <100%> (ø) ⬆️
src/nodes/DoWhileStatement.js 100% <100%> (ø) ⬆️
src/nodes/Block.js 100% <100%> (ø) ⬆️
src/nodes/FunctionTypeName.js 100% <100%> (ø) ⬆️
src/nodes/AssemblyCase.js 100% <100%> (ø) ⬆️
src/nodes/IfStatement.js 100% <100%> (ø) ⬆️
src/nodes/ReturnStatement.js 100% <100%> (ø) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 794f8a6...016c2be. Read the comment docs.

Copy link
Member

@mattiaerre mattiaerre left a comment

Choose a reason for hiding this comment

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

I really like it, thanks @Janther

Copy link
Member

@fvictorio fvictorio left a comment

Choose a reason for hiding this comment

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

I usually find ternary operators harder to read, but I guess I'll get used to them.

@mattiaerre
Copy link
Member

Oh, that's a good point @fvictorio I don't wanna compromise readability. But in this case, I'm neutral. What should we be doing?

@fvictorio
Copy link
Member

I'm mostly neutral too, so I'm fine with merging it.

@mattiaerre mattiaerre merged commit 32cac96 into master Feb 29, 2020
@mattiaerre mattiaerre deleted the refactor branch February 29, 2020 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor reorganising the code without changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants