Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Cleanup #715

Merged
merged 5 commits into from Dec 27, 2012

Conversation

Projects
None yet
4 participants
Contributor

carols10cents commented Nov 13, 2012

These are the commits from @gavinlaking's pull request having to do with generic cleanup. Gavin, I kept you as the author of these commits even though I made some modifications-- I'd love if you had some time to look this over to make sure I captured your intent accurately ❤️ ❤️ ❤️

@carols10cents carols10cents referenced this pull request Nov 17, 2012

Closed

Fixing Issue 698 #708

Contributor

carols10cents commented Nov 17, 2012

I personally find trailing conditionals to be less than clear... but I'm on the fence about the ternary operator.

@wilkie, how would you feel about:

def display_name
  if name.present?
    name
  else
    username
  end
end
Contributor

wilkie commented Nov 17, 2012

Isn't that the original code? :P

You don't like trailing conditionals? Is that a common opinion?

I don't mind the longhand form... the ternary ops are just short for the sake of being short. What do you think is more readable? I want an opinion of why. Readability is what we should promote.

Contributor

steveklabnik commented Nov 17, 2012

You don't like trailing conditionals? Is that a common opinion?

It's a minority opinion, but it's a large-ish minority.

Rubyists almost never use ?: unless it's very short. I'm fine in this case, but I wouldn't prefer it over if/else either.

Contributor

carols10cents commented Nov 18, 2012

Haha yes that's the original code. @gavinlaking proposed the change, I'm fine with the original ;)

I personally find trailing conditionals to be like giving instructions and THEN saying "oh but you dont have to do any of this in your case"-- you've just wasted the reader's time and brainpower. it feels like the wrong order to me. That impacts readability in my mind. But I'm willing to be the overruled minority :)

Contributor

wilkie commented Nov 21, 2012

I agree with you: I don't mind the original either. I find it tremendously easier to read. I don't mind the trailing conditionals because I've grown to look for them. But I like them less. I write verbose code, and I don't feel there is anything wrong with that. :)

Contributor

carols10cents commented Nov 22, 2012

Rereview time!!!

carols10cents added a commit that referenced this pull request Dec 27, 2012

@carols10cents carols10cents merged commit 2120fb6 into hotsh:master Dec 27, 2012

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment