# Is my code unelegant?

**URL:** <https://www.ruby-forum.com/t/is-my-code-unelegant/194431>\
**Category:** Ruby\
**Created:** [September 5, 2010, 11:49pm UTC](https://www.ruby-forum.com/t/is-my-code-unelegant/194431 "2010-09-05T23:49:05Z")\
**Posts on this page:** 4\
**Page:** 1

<div class="post-metadata">

**Author:** ![marksbrain](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/m/6bbea6/32.png) [@marksbrain](https://www.ruby-forum.com/u/marksbrain)\
**Post date:** [September 5, 2010, 11:49pm UTC](https://www.ruby-forum.com/t/is-my-code-unelegant/194431/1 "2010-09-05T23:49:05Z")

</div>

Is the attached code too messy to be considered “good”?

I can’t figure out if I created too many variables.

I’m just now learning Ruby, and I just can’t figure out if that’s bad  
code.

---

<div class="post-metadata">

**Author:** ![marksbrain](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/m/6bbea6/32.png) [@marksbrain](https://www.ruby-forum.com/u/marksbrain)\
**Post date:** [September 5, 2010, 11:51pm UTC](https://www.ruby-forum.com/t/is-my-code-unelegant/194431/2 "2010-09-05T23:51:22Z")

</div>

I should add that the point of this program is to have a user enter a  
number between 1 and 3000 and it will convert it to Old Roman Numerals.

---

<div class="post-metadata">

**Author:** ![marksbrain](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/m/6bbea6/32.png) [@marksbrain](https://www.ruby-forum.com/u/marksbrain)\
**Post date:** [September 6, 2010, 4:39pm UTC](https://www.ruby-forum.com/t/is-my-code-unelegant/194431/3 "2010-09-06T16:39:33Z")

</div>

One immediate modification would be to replace the final two lines of  
your roman\_numerals() method with:

(‘M’ \* number\_of\_m) + (‘D’ \* number\_of\_d) + (‘C’ \* number\_of\_c) + (‘L’

- number\_of\_l) + (‘X’ \* number\_of\_x) + (‘V’ \* number\_of\_v) + (‘I’ \*  
number\_of\_i)

since the method will return that without the need for an explicit  
return statement.

You’ll notice that there is no puts in this, either. Your method  
should return a string. Creating the roman numeral version of the  
given number is a separate concern to actually printing that number.  
You’re better off then using puts roman\_numerals(argument).

Just looking at it, I see nothing which restricts the input to between  
1 and 3000.

---

<div class="post-metadata">

**Author:** ![marksbrain](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/m/6bbea6/32.png) [@marksbrain](https://www.ruby-forum.com/u/marksbrain)\
**Post date:** [September 6, 2010, 4:39pm UTC](https://www.ruby-forum.com/t/is-my-code-unelegant/194431/4 "2010-09-06T16:39:38Z")

</div>

On Sep 5, 4:49 pm, Mark W. [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid) wrote:

> Is the attached code too messy to be considered “good”?
> 
> I can’t figure out if I created too many variables.
> 
> I’m just now learning Ruby, and I just can’t figure out if that’s bad  
> code.
> 
> Attachments:[http://www.ruby-forum.com/attachment/5007/romannumerals1.rb](http://www.ruby-forum.com/attachment/5007/romannumerals1.rb)
> 
> –  
> Posted viahttp://www.ruby-forum.com/.

def old\_roman n  
[1000,500,100,50,10,5,1].  
map{|d| quot, n = n.divmod(d); quot }.  
zip( %w(M D C L X V I) ).  
map{|n,c| c \* n}.join  
end

puts "Enter a number and I’ll convert " +  
“it to the older Roman Numerals!”  
puts old\_roman( gets.to\_i )