Design review for classes and mixins

ruby-talk
1

I've been refactoring a class as I re-read "Practical Object Oriented
Design in Ruby". Moving from "one class to rule them all" to as much
decoupling as possible. Looking for some critique of my new design.

https://github.com/LeamHall/CT_Character_Generator/tree/refactor_character

The base class, Character, stores various attributes about a Traveller(tm)
character. Stats, age, skills, etc. Right now the plan is to store that in
JSON.

There are a couple of Modules I'm working on. One (Traveller.rb), sort of a
Factory, that can set or change attributes. It can also be used stand-alone
when I need to generate things like a list of random names
(lib/get_names.rb). This one is mostly written.

I'm also looking at a Presenter class. One that will take the data and
present it in a variety of ways.

From a basic design perspective, am I on the right track? What could be
done better?

Leam

···

--
Mind on a Mission <http://leamhall.blogspot.com/>

2

One quick note - there's no need to define all your module functions as
"def Traveller.roll_dice". See
Idiosyncratic Ruby: Self Improvement for other options.

martin

···

On Wed, Jul 6, 2016 at 7:38 AM, leam hall <leamhall@gmail.com> wrote:

I've been refactoring a class as I re-read "Practical Object Oriented
Design in Ruby". Moving from "one class to rule them all" to as much
decoupling as possible. Looking for some critique of my new design.

https://github.com/LeamHall/CT_Character_Generator/tree/refactor_character

The base class, Character, stores various attributes about a Traveller(tm)
character. Stats, age, skills, etc. Right now the plan is to store that in
JSON.

There are a couple of Modules I'm working on. One (Traveller.rb), sort of
a Factory, that can set or change attributes. It can also be used
stand-alone when I need to generate things like a list of random names
(lib/get_names.rb). This one is mostly written.

I'm also looking at a Presenter class. One that will take the data and
present it in a variety of ways.

From a basic design perspective, am I on the right track? What could be
done better?

Leam

--
Mind on a Mission <http://leamhall.blogspot.com/&gt;

Unsubscribe: <mailto:ruby-talk-request@ruby-lang.org?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-talk&gt;

3

Some suggestions based on your traveller module:

1. Rather than inspecting the members of Character within Traveller.write,
have methods #to_text, #to_csv, #to_html etc. within the Character class,
returning string representations. Then Traveller.write would be:

def Traveller.write(c, mode):
  case mode:
    when 'text':
      puts c.to_text
    when 'csv':
      puts c.to_csv
    # etc.

Furthermore, mode = 'hash' does not belong in the 'write' method since it
returns a hash and does not write anything, whereas all the others write to
stdout but return nothing.

2. If you define skills = Hash.new(0) you can remove the
Traveller.add_skill method altogether and just say skills[skill] += level.

Demonstration:

a = Hash.new(0)

=> {}

a['foo'] += 10

=> 10

a

=> {"foo"=>10}

a['foo'] += 10

=> 20

a

=> {"foo"=>20}

3. Refactor Traveller.first_name and Traveller.last_name:

a. have a get_name_from_db(table) method

b. rewrite your Traveller.name method to be

def Traveller.make_name(gender)
  first_name_table = "humaniti_#{gender}_first"
  last_name_table = "humaniti_last"
  first_name = Traveller.get_name_from_db(first_name_table)
  last_name = Traveller.get_name_from_db(last_name_table)
  return "#{first_name} #{last_name}"
end

4. Traveller.roll_dice and Traveller.upp should not be using instance
variables.

martin

···

On Wed, Jul 6, 2016 at 7:38 AM, leam hall <leamhall@gmail.com> wrote:

I've been refactoring a class as I re-read "Practical Object Oriented
Design in Ruby". Moving from "one class to rule them all" to as much
decoupling as possible. Looking for some critique of my new design.

https://github.com/LeamHall/CT_Character_Generator/tree/refactor_character

The base class, Character, stores various attributes about a Traveller(tm)
character. Stats, age, skills, etc. Right now the plan is to store that in
JSON.

There are a couple of Modules I'm working on. One (Traveller.rb), sort of
a Factory, that can set or change attributes. It can also be used
stand-alone when I need to generate things like a list of random names
(lib/get_names.rb). This one is mostly written.

I'm also looking at a Presenter class. One that will take the data and
present it in a variety of ways.

From a basic design perspective, am I on the right track? What could be
done better?

Leam

--
Mind on a Mission <http://leamhall.blogspot.com/&gt;

Unsubscribe: <mailto:ruby-talk-request@ruby-lang.org?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-talk&gt;

4

Martin, thanks! Let me add some to the discussion. Notes are in-line.

Some suggestions based on your traveller module:

1. Rather than inspecting the members of Character within Traveller.write,
have methods #to_text, #to_csv, #to_html etc. within the Character class,
returning string representations. Then Traveller.write would be:

def Traveller.write(c, mode):
  case mode:
    when 'text':
      puts c.to_text
    when 'csv':
      puts c.to_csv
    # etc.

Furthermore, mode = 'hash' does not belong in the 'write' method since it
returns a hash and does not write anything, whereas all the others write to
stdout but return nothing.

What I wasn't sure about was the HTML output. If there's a method in the
Class then how do you allow for text formatting if the web page wants to do
bold or other text formatting? Not all characters will have all fields, and
not all use cases will need all data.

2. If you define skills = Hash.new(0) you can remove the
Traveller.add_skill method altogether and just say skills[skill] += level.

Demonstration:

> a = Hash.new(0)
=> {}
> a['foo'] += 10
=> 10
> a
=> {"foo"=>10}
> a['foo'] += 10
=> 20
> a
=> {"foo"=>20}

Ah, much nicer! Thanks!

3. Refactor Traveller.first_name and Traveller.last_name:

a. have a get_name_from_db(table) method

b. rewrite your Traveller.name method to be

def Traveller.make_name(gender)
  first_name_table = "humaniti_#{gender}_first"
  last_name_table = "humaniti_last"
  first_name = Traveller.get_name_from_db(first_name_table)
  last_name = Traveller.get_name_from_db(last_name_table)
  return "#{first_name} #{last_name}"
end

Let me ponder this. There are eventually supposed to be several more tables
for names, based on culture. Some cultures have very different naming
schemes.

4. Traveller.roll_dice and Traveller.upp should not be using instance
variables.

Thanks! I'll fix this shortly.

···

On Thu, Jul 7, 2016 at 8:45 PM, Martin DeMello <martindemello@gmail.com> wrote:

--
Mind on a Mission <http://leamhall.blogspot.com/&gt;

5

Martin, I tried this:

"2. If you define skills = Hash.new(0) you can remove the
Traveller.add_skill method altogether and just say skills[skill] += level. "

And it works great! That will simplify my code in a few places.

···

--
Mind on a Mission <http://leamhall.blogspot.com/>

6

Martin, thanks! Let me add some to the discussion. Notes are in-line.

Some suggestions based on your traveller module:

1. Rather than inspecting the members of Character within
Traveller.write, have methods #to_text, #to_csv, #to_html etc. within the
Character class, returning string representations. Then Traveller.write
would be:

def Traveller.write(c, mode):
  case mode:
    when 'text':
      puts c.to_text
    when 'csv':
      puts c.to_csv
    # etc.

Furthermore, mode = 'hash' does not belong in the 'write' method since it
returns a hash and does not write anything, whereas all the others write to
stdout but return nothing.

What I wasn't sure about was the HTML output. If there's a method in the
Class then how do you allow for text formatting if the web page wants to do
bold or other text formatting? Not all characters will have all fields, and
not all use cases will need all data.

Okay, it sounds like you want the Character class to be a pure data
container, which is a good design too. But if you want to do that, then
just access its members directly everywhere, there's no need to do things
like

rank = c.rank

I'd suggest making it a struct [
http://culttt.com/2015/04/15/working-with-structs-in-ruby/\] - as a bonus
you get a to_h method for free

Also, you should still separate the concerns of formatting the character
data as a string, and outputting the string. In the specific case of web
page formatting, the usual way is to use a template - see
http://www.stuartellis.eu/articles/erb/

You could have a CharacterFormatter class with methods for each of the
different formats, and then call the appropriate method from Traveller.write

3. Refactor Traveller.first_name and Traveller.last_name:

a. have a get_name_from_db(table) method

b. rewrite your Traveller.name method to be

def Traveller.make_name(gender)
  first_name_table = "humaniti_#{gender}_first"
  last_name_table = "humaniti_last"
  first_name = Traveller.get_name_from_db(first_name_table)
  last_name = Traveller.get_name_from_db(last_name_table)
  return "#{first_name} #{last_name}"
end

Let me ponder this. There are eventually supposed to be several more
tables for names, based on culture. Some cultures have very different
naming schemes.

You should still separate out the logic of picking a random row from a
database table into its own method.

martin

···

On Fri, Jul 8, 2016 at 7:39 AM, leam hall <leamhall@gmail.com> wrote:

On Thu, Jul 7, 2016 at 8:45 PM, Martin DeMello <martindemello@gmail.com> > wrote:

7

Great :slight_smile: Explore the ruby standard library a bit, there's a lot of neat
stuff already in there that you don't need to reimplement.

martin

···

On Thu, Jul 14, 2016 at 5:38 AM, leam hall <leamhall@gmail.com> wrote:

Martin, I tried this:

"2. If you define skills = Hash.new(0) you can remove the
Traveller.add_skill method altogether and just say skills[skill] += level. "

And it works great! That will simplify my code in a few places.

--
Mind on a Mission <http://leamhall.blogspot.com/&gt;

Unsubscribe: <mailto:ruby-talk-request@ruby-lang.org?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-talk&gt;

8

Martin, just wanted to let you know I appreciate the gentle reminder to read more. I have the Pickaxe book and have been reading the "Ruby Library Reference" sections while waiting for our internet service to get plugged in.

Some of it is mind-numbing. Some is over my head. However, this first light read is mostly to start building the data structures in my head so I can later go back and study more about the bits I need. Many of the things I didn't even know about.

For me personally, it's a turning point. I've been able to use a few languages based on the first chapters of a book or web tutorials. I think this is the first time I've actually gotten into the reference sections to learn that depth. Hopefully it will make me a better programmer, and a better Ruby Programmer.

Leam

···

On 07/14/16 15:27, Martin DeMello wrote:

Great :slight_smile: Explore the ruby standard library a bit, there's a lot of neat
stuff already in there that you don't need to reimplement.

martin