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

cache_req: Use ternary operator instead of bool+switch #250

Closed
wants to merge 1 commit into from

Conversation

@lslebodn
Copy link
Contributor

lslebodn commented Apr 27, 2017

IMHO it looks little bit better.
I know there is explanation but would like to see other opinion. @fidencio what do you think?

@fidencio

This comment has been minimized.

Copy link
Contributor

fidencio commented Apr 27, 2017

@lslebodn, I've chosen the switch just to make the code cleaner, as I do believe the cases are easier to read than the ternary operator.

However, if you think it's easier to read using the ternary operator, I won't oppose to it.

@lslebodn

This comment has been minimized.

Copy link
Contributor Author

lslebodn commented Apr 27, 2017

I think there is a big difference between 11 lines and 3 lines.
switch + boolean does not simplify it.

It might improve readability a bit in case of complicated boolean expression. But it does not simplify it in this case (true/false/value of variable)

But I would like to see more opinions.

@pbrezina

This comment has been minimized.

Copy link
Member

pbrezina commented Apr 27, 2017

I, personally, would have used if/else. But between these two, I found switch more readable then nested ternary operator.

@mzidek-gh

This comment has been minimized.

Copy link
Contributor

mzidek-gh commented Apr 27, 2017

What about:

     if (head->fqnames) {
         return true;
     } else if (enforce_non_fqnames) {
         return false;
     } else {
         return domain->fqnames;
     }

I also do not think the switch is very good choice in case of boolean true/false values. I am Ok with the Lukáš's version, but I like the "if - else if - else" version even better.

@fidencio

This comment has been minimized.

Copy link
Contributor

fidencio commented Apr 27, 2017

Personally I think any of the suggestions make the code more readable than the switch (which has all the cases explicitly matching with the comments above).

For me, it's a nack for the patch and for those suggestions.

Lukas Slebodnik
Pair-Programmed-With: Michal Židek <mzidek@redhat.com>
@lslebodn

This comment has been minimized.

Copy link
Contributor Author

lslebodn commented Apr 27, 2017

I Personally I think any of the suggestions make the code less readable than the switch (which as all the cases explicitly matching with the comments above).

I'm sorry but switch is the worst option from proposed (IMHO)

It looks like 2 guys like version with if. I will prepare version with if even though that ternary is more readable :-). Democracy works.

@lslebodn lslebodn force-pushed the lslebodn:bool_switch branch from eb262ff to 98bdb06 Apr 27, 2017
@fidencio

This comment has been minimized.

Copy link
Contributor

fidencio commented Apr 27, 2017

I still don't think it makes anything cleaner, just reduce the amount of lines.

It's a nack from me, but if someone else feels like acking it ... I won't oppose either.

@lslebodn

This comment has been minimized.

Copy link
Contributor Author

lslebodn commented Apr 27, 2017

I still don't think it makes anything cleaner, just reduce the amount of lines.

The main purpose is not reduction of lines. But make it more readable. After the discussion, it was decided that switch+bool is the less readable form proposed options. (just you like it :-)

@fidencio

This comment has been minimized.

Copy link
Contributor

fidencio commented Apr 28, 2017

No, it was decided that just other people like the if more, not that it is more readable.

Remember that forcing your opinion on a NACK will be taken into consideration when you'll be NACKing something ... (which is fine by me, but may not be for you).

@lslebodn

This comment has been minimized.

Copy link
Contributor Author

lslebodn commented Apr 28, 2017

No, it was decided that just other people like the if more, not that it is more readable.

If they they like it more then it is more readable.

Remember that forcing your opinion on a NACK will be taken into consideration when you'll be NACKing something ... (which is fine by me, but may not be for you).

If I saw switch + bool earlier you would request change as part of review. But I do not have a resources to review all patches these days due to other tasks. And in this case, your NACK does not count because there are 3 other developers which prefer different version. (3 vs 1) Sorry democracy works. The same applies to any other developer(including me) NACK cannot beet 3+ developers :-)
Because reasonable NACK with proper explanation would persuade them.

@fidencio

This comment has been minimized.

Copy link
Contributor

fidencio commented Apr 28, 2017

Okay, I agree that democracy should win, in all cases :-)

I'm acking the patch and keeping this PR on track for future uses where the basic democratic rule is not used.

Please, run the CI on this very simple patch before pushing it.

@fidencio fidencio added the Accepted label Apr 28, 2017
@mzidek-gh

This comment has been minimized.

Copy link
Contributor

mzidek-gh commented Apr 28, 2017

LGTM++

@lslebodn lslebodn closed this Apr 28, 2017
@lslebodn lslebodn deleted the lslebodn:bool_switch branch Apr 28, 2017
@lslebodn

This comment has been minimized.

Copy link
Contributor Author

lslebodn commented Apr 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.