The Better Code

Ruby
1

Which would you judge to be the better code?

The concise:

def self.assert(*a, &b)
  o = Hash === a.last ? a.pop : {}

  t = o[:backtrace] || caller
  m = o[:message]

  e = new(m, *a, &b)
  e.set_backtrace(t)
  e.assert
end

Or the more explicit:

def self.assert(*arguments, &block)
  options = (Hash === arguments.last ? arguments.pop : {})

  backtrace = options[:backtrace] || caller
  message   = options[:message]

  assay = new(message, *arguments, &block)
  assay.set_backtrace(backtrace)
  assay.assert
end

Which and why?

2

+1 for the more verbose version. Same reasons as Adam P…

3

Anything with this line in it can’t be considered “better” than anything
else :slight_smile:

4

The latter. Because spending a little extra more time to make things
more
readable will make other people (experienced and inexperienced alike),
along with your future self, have a more pleasant time following what’s
happening. Optimise happiness, not keystrokes.

5

Well, the second is clearer to read line by line. But I find it easier
to grasp at a glance what the first one is doing as there’s less noise
from variable names. A middle way of course would be to use
abbreviated variable names, such as ‘args’, ‘opts’, etc.

6

The second one. Much easier to understand because it is explicit.

7

On 15 Jan 2012, at 6:30 PM, Intransition wrote:

Which would you judge to be the better code?

The concise:

def self.assert(*a, &b)
  o = Hash === a.last ? a.pop : {}

…

Or the more explicit:

def self.assert(*arguments, &block)
  options = (Hash === arguments.last ? arguments.pop : {})

Quoting IsaacSchlueter:

8

On Mon, Jan 16, 2012 at 09:30:47AM +0900, Intransition wrote:

Which and why?

It’s not just a matter of concise vs. explicit code. If you can make it
shorter without losing the meaningful and clear structure, and the
meaningful labels for things, shorter is generally better because it’s
easier to take in at a glance. In this case, however, the so-called
“concise” option is gratuitously minimal in terms of the naming
convention for some of the labels. The one-letter variables are not
block argument variables, and the letters do not at a glance appear
meaningful at all. As such, it’s obfuscated rather than merely concise.

By contrast, in RPG culture, the letter “d” has a long history of clear
semantic value as an abbreviation for “die” or “dice”. The same is true
of “str”, “dex”, “con”, “int”, “wis”, and “cha”, as abbreviations for
other terms. These may appear a little odd to those coming from outside
that culture, but for people familiar with it they are about as clear as
can be, and even clearer than the full-length names in a lot of
potential
uses within source code. In software I maintain for managing RPG
character stats and handling virtual die rolls, these abbreviations
clarify the code significantly. By the same token, when you have an
array called “filenames” and want to iterate over its contents,
“filenames.each do {|f| do.something_with(f) }” can be perfectly clear.

So . . . it depends. Clarity trumps rules of thumbs. When in doubt,
though, give the reader a sense of what something is when you name it,
and try to avoid making your code look like Snoopy swearing (regexen
notwithstanding).

9

On Sun, Jan 15, 2012 at 6:30 PM, Intransition [email protected]
wrote:

  e = new(m, *a, &b)
  message   = options[:message]

  assay = new(message, *arguments, &block)
  assay.set_backtrace(backtrace)
  assay.assert
end

Which and why?

Explicit, because I was able to understand it. On the first one, when I
got
to the new, I was confused about the params being passed in. I also like
how it uses parens on the ternary during assignment, because otherwise
the
syntax is confusing. I don’t think it needs the parens after new and
set_backtrace, my preference is the minimum amount of syntax such that
the idea is clear.

As an asside, it’s unclear to me why it’s set_backtrace rather than
backtrace=

10

On Mon, Jan 16, 2012 at 2:20 AM, Adam P. [email protected]
wrote:

The latter. Because spending a little extra more time to make things more
readable will make other people (experienced and inexperienced alike),
along with your future self, have a more pleasant time following what’s
happening. Optimise happiness, not keystrokes.

+1

Plus, I’d rid of variable “message” because it is used in one place
only and removing does not loose any documentation relevant
information. Maybe I’d also remove “backtrace” because that is used
in one place only also.

Doesn’t look too bad, does it?

def self.assert(*arguments, &block)
  options = (Hash === arguments.last ? arguments.pop : {})

  assay = new(options[:message], *arguments, &block)
  assay.set_backtrace(options[:backtrace] || caller)
  assay.assert
end

A variable always introduces one step in reading indirection which is
only warranted if the value is used in several places or not using the
variable clutters expressions too much.

Kind regards

robert

11

On Sunday, January 15, 2012 10:24:38 PM UTC-5, Gavin S. wrote:

Anything with this line in it can’t be considered “better” than anything
else :slight_smile:

  o = Hash === a.last ? a.pop : {}

I concur with that! You have an alternative?

Thankfully Ruby 2.0 will (finally) will make it obsolete.

12

Some very interesting comments. It would appear that a certain
contingent
are for a middle way, something like:

def self.assert(*args, &blk)
  opts = (Hash === args.last ? args.pop : {})

  bt  = opts[:backtrace] || caller
  msg = opts[:message]

  err = new(msg, *args, &blk)
  err.set_backtrace(bt)
  err.assert
end

I find myself doing that more often myself, but when I later revisit the
code tend to expand it to long form.

Yet, given some of the arguments, I wonder if using in-line comments
along
with the super-concise version would not be a good “middle” approach
too:

def self.assert(*a, &b)
  o = (Hash === a.last ? a.pop : {})  # options

  t = o[:backtrace] || caller  # backtrace
  m = o[:message]              # message

  e = new(m, *a, &b)           # assay exception
  e.set_backtrace(t)
  e.assert
end
13

On Mon, Jan 16, 2012 at 15:57, Intransition [email protected] wrote:

Yet, given some of the arguments, I wonder if using in-line comments along
with the super-concise version would not be a good “middle” approach too:

I think I’d rather just have the code speak for itself with meaningful
variable names. Otherwise you’re instead reading comments to describe
code
that could’ve described itself! Besides, in this case, :backtrace and
:message give the same information as the comments.

14

On Mon, Jan 16, 2012 at 5:02 PM, Adam P. [email protected]
wrote:

On Mon, Jan 16, 2012 at 15:57, Intransition [email protected] wrote:

Yet, given some of the arguments, I wonder if using in-line comments along
with the super-concise version would not be a good “middle” approach too:

I think I’d rather just have the code speak for itself with meaningful
variable names. Otherwise you’re instead reading comments to describe code
that could’ve described itself! Besides, in this case, :backtrace and
:message give the same information as the comments.

+2

Cheers

robert

15

On Tue, Jan 17, 2012 at 01:02:42AM +0900, Adam P. wrote:

I think I’d rather just have the code speak for itself with meaningful
variable names. Otherwise you’re instead reading comments to describe code
that could’ve described itself! Besides, in this case, :backtrace and
:message give the same information as the comments.

. . . and, on top of everything else, if you have to change the code
you’re going to have to change the comments too. When your comments end
up looking like pseudocode that duplicates the “real” code, you’re not
doing anything that shouldn’t be done by just writing clearer code.