I know this code is not very 'ruby'

[Tim Booher]

This function should work, but when I wrote it, I had the feeling that
there would be a much better way to code this function. In particular, I
don't let creating the discount_out variable. anyone want to give me a
nudge in the right direction?

def discount(discount_category)
# 0 => no discount
# 1 => friend
# 2 => vet
# 3 => super-vet
case discount_category
when 1
discount_out = 40
when 2
discount_out = 100
when 3
discount_out = 160
else
discount_out = 0
end
discount_out
end

best,

tim

 

[Sandro Paganotti]

Compressed version

discount_out = [0,40,100,160][discount_category].to_i

 

[Eleanor McHugh]

This function should work, but when I wrote it, I had the feeling that
there would be a much better way to code this function. In
particular, I
don't let creating the discount_out variable. anyone want to give me a
nudge in the right direction?

def discount(discount_category)
# 0 => no discount
# 1 => friend
# 2 => vet
# 3 => super-vet
case discount_category
when 1
discount_out = 40
when 2
discount_out = 100
when 3
discount_out = 160
else
discount_out = 0
end
discount_out
end

Remember that case statements are expressions, hence:

def discount(discount_category)
case discount_category
when 1 : 40
when 2 : 100
when 3 : 160
else discount_out
end
end

will result in the chosen value be returned by the function.

For clarity you also might like to pretty this up further with some
symbols:

def discount discount_category = :no_discount
case discount_category
when :friend : 40
when :vet : 100
when :super_vet : 160
else discount_out
end
end

then you don't need the comments to document your code.


Ellie

Eleanor McHugh
Games With Brains
http://slides.games-with-brains.net

 

[Todd Benson]

This function should work, but when I wrote it, I had the feeling that
there would be a much better way to code this function. In particular, I
don't let creating the discount_out variable. anyone want to give me a
nudge in the right direction?

def discount(discount_category)
# 0 => no discount
# 1 => friend
# 2 => vet
# 3 => super-vet
case discount_category
when 1
discount_out = 40
when 2
discount_out = 100
when 3
discount_out = 160
else
discount_out = 0
end
discount_out
end

best,

tim

You already have a hash structure...


def discount category
#I'll set up the hash for demonstration
#You would probably use class instance
#variables instead, or a const for your
#id to category mapping outside of the def
(h = Hash[1, 40, 2, 100, 3, 160]).default = 0

#grab the category
h[category]
end


...in which case you could do away with the def altogether depending
on what you're trying to do.

Todd

 

[Robert Klemme]

Remember that case statements are expressions, hence:

def discount(discount_category)
case discount_category
when 1 : 40
when 2 : 100
when 3 : 160
else discount_out

I guess this should read

else 0

end
end

will result in the chosen value be returned by the function.

For clarity you also might like to pretty this up further with some
symbols:

def discount discount_category = :no_discount
case discount_category
when :friend : 40
when :vet : 100
when :super_vet : 160
else discount_out
end
end

then you don't need the comments to document your code.

I was going to suggest something similar, i.e. use more verbose
category "names". If categories are read from somewhere they could be
mapped on input.

But, as Todd also mentions, this cries for a Hash. With a Hash method
#discount is basically superfluous.

DISCOUNT = Hash.new(0).update(
:friend => 40,
:vet => 100,
:super_vet => 160
).freeze

Then a method call essentially becomes

DISCOUNT[:foo]

instead of

discount :foo

Kind regards

robert

 

[Phillip Gawlowski]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Robert Klemme wrote:

|
| But, as Todd also mentions, this cries for a Hash. With a Hash method
| #discount is basically superfluous.
|
| DISCOUNT = Hash.new(0).update(
| :friend => 40,
| :vet => 100,
| :super_vet => 160
| ).freeze
|
| Then a method call essentially becomes
|
| DISCOUNT[:foo]

I don't like this.

| instead of
|
| discount :foo

But I like that.

So, let's do a Classic version (pardon the pun):

class Discounts

~ attr_reader :friend, :vet, :super_vet


~ def initialize
~ @friend, @vet, @super_vet = 40, 100, 160
~ end

~ def method_missing
~ 0 # The default 'discount'
~ end
end

discount_for = Discounts.new


discount_for vet
=> 100

discount_for somebody_else
=> 0


Beware of bugs; I've only proved the code correct, not tested it.

- --
Phillip Gawlowski
Twitter: twitter.com/cynicalryan
Blog: http://justarubyist.blogspot.com

~ - You know you've been hacking too long when...
...you discover that you're balancing your checkbook in octal.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkggeIYACgkQbtAgaoJTgL9FBwCfbEkHYMGYwGyyjig79uEtpJEJ
pt8AniofvYQwftnufso/za+DfgfTPNLY
=+K4V
-----END PGP SIGNATURE-----

 

[Eleanor McHugh]

I guess this should read

else 0

That's what happens when I write code before my second cup of tea of
the morning

I was going to suggest something similar, i.e. use more verbose
category "names". If categories are read from somewhere they could be
mapped on input.

But, as Todd also mentions, this cries for a Hash. With a Hash method
#discount is basically superfluous.

DISCOUNT = Hash.new(0).update(
:friend => 40,
:vet => 100,
:super_vet => 160
).freeze

Then a method call essentially becomes

DISCOUNT[:foo]

instead of

discount :foo

I'd probably use a Hash as well for a trivial mapping like this, but I
thought it was more instructive to just abolish the unnecessary
variable seeing as Tim clearly didn't realise that Ruby uses a case
expression rather than a case statement. And of course if the discount
were to rely on several parameters then the case expression form might
well be much easier to maintain.


Ellie

Eleanor McHugh
Games With Brains
http://slides.games-with-brains.net

 

[Phillip Gawlowski]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Phillip Gawlowski wrote:

|
| So, let's do a Classic version (pardon the pun):
|
| class Discounts
|
| ~ attr_reader :friend, :vet, :super_vet
|
|
| ~ def initialize
| ~ @friend, @vet, @super_vet = 40, 100, 160
| ~ end
|
| ~ def method_missing
| ~ 0 # The default 'discount'
| ~ end
| end
|
| discount_for = Discounts.new
|
|
| discount_for vet
| => 100
|
| discount_for somebody_else
| => 0
|
|
| Beware of bugs; I've only proved the code correct, not tested it.

Well, not even that bit of due diligence. *sighs*

Obviously, that doesn't work.

discount_for.vet # that does work with the above code.

However, defining a singleton method would provide what I like to see.

def discounts_for buyer

~ case buyer
~ when friend : 40
~ .
~ .
~ .
~ else 0
~ end
end


Or define it as a class method:

class Discounts
~ # Handling of variables left out as exercise for the reader
~ def self.for
~ case[...]
~ end
end


That way you could call, for example:


Discounts.for vet


- --
Phillip Gawlowski
Twitter: twitter.com/cynicalryan
Blog: http://justarubyist.blogspot.com

~ - You know you've been hacking too long when...
...you send E-mail and end each line with \n.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEUEARECAAYFAkgggrwACgkQbtAgaoJTgL+aQgCWMqrQV0LqpvMEdFCb1yCluEKg
bACgnmMR8XvQzfoYalnQiH9ra3teVL4=
=hs10
-----END PGP SIGNATURE-----