2
\$\begingroup\$

I have made 2 scripts in Ruby. The first program creates an .rtf file with cards (file with colored table with persons) from a list of people in text file. The second program does the same, but in an HTML file. In the first, I can use the gem library, but I'm not allowed to do so in the second.

My teacher said that in both cases, code could be two times shorter. He also said that I'm not organising stuff in class and methods; that could be a problem if I start to write more advanced codes in future. I'm asking where and how I could improve my code to be shorter and organised in classed/method. I placed my notes in the comments.

require 'rubygems'
require 'rtf'
include RTF

def card(t, person, style1, style2, i)
    style = CharacterStyle.new
    x = i/2.0.floor
    y = i % 2
    if x % 2 == 0
        style = style1
        t[x][y].shading_colour = $olive
    else
        style = style2
        t[x][y].shading_colour = $maroon
    end
    person = person.split(";")
    # I could bet that there's some way to automatically apply line_break command after all following lines.

    t[x][y].apply(style) << "name: " + person[0]
    t[x][y].line_break
    t[x][y].apply(style) << "surname: " + person[1]
    t[x][y].line_break
    t[x][y].apply(style) << "date_of_birth: " + person[2]
    t[x][y].line_break
    t[x][y].apply(style) << "salary: " + person[3]
    t[x][y].line_break
    t[x][y].apply(style) << "phone: " + person[4]
    t[x][y].line_break
    t[x][y].apply(style) << "position: " + person[5]
    t[x][y].line_break
    t[x][y].apply(style) << "email: "+ person[6]
end 

document = Document.new(Font.new(Font::ROMAN, 'Times New Roman'), nil, Document::CS_PC, Document::LC_POLISH)

# I don't like using a global variables there but applying them directly in method card would slow down whole script. 

$maroon = Colour.new(125, 0, 0)
$olive = Colour.new(125, 125, 0)
style1 = CharacterStyle.new
style1.font = Font.new(Font::MODERN, 'Calibri')
style1.foreground = $maroon
style1.underline = true
style2 = CharacterStyle.new
style2.font = Font.new(Font::ROMAN, 'Times New Roman')
style2.bold = true
style2.foreground = $olive
people = []
f = open('base.txt', "r:UTF-8") { |f| f.read }
f.each_line { |ln| people << ln }
array = document.table(people.length/2.0.ceil,2, 4000,4000,4000)

# I don't like using "i" variable as cell counter of table. I would like to use some useful method instead of that.

i = 0
people.each {|person|
    card(array, person, style1, style2, i)
    i += 1
}
File.open('cards.rtf', 'w:UTF-8') {|f| f.write(document.to_rtf)}

Second program

def b(text)
    "<b>#{text}</b>"
end

def array(document, person, i)
    person = person.split(';')

    # I don't like the "i" variable. I think there is an other simple way in ruby to do something diffrent in loop for every second element.

    if i % 2 == 0
        document << "<tr>"
        color1 = 'maroon'
        color2 = 'olive'
    else
        color1 = 'olive'
        color2 = 'maroon'
    end

    # There would be great if i could organise array styles in methods.

    document << "<td><p style='background-color:#{color1};font-family:arial;color:#{color2};font-size:20px;'>"
    document << b("name: ") + person[0] + "<br />"
    document << b("surname: ") + person[1] + "<br />"
    document << b("date_of_birth: ") + person[2] + "<br />"
    document << b("salary: ") + person[3] + "<br />"
    document << b("phone: ") + person[4] + "<br />"
    document << b("position: ") + person[5] + "<br />"
    document << b("email: ") + person[6] + "<br />"
    document << "</p></td>"
    if i % 2 == 1
        document << "</tr>"
    end
end 

people = []
f = open('base.txt', "r:UTF-8") { |f| f.read }
f.each_line { |ln| people << ln }
document = ''

# I would like to create document as an object of some class.

document << 
    "<!DOCTYPE html PUBLIC '-//W3C//DTD HTML 4.01 Transitional//EN'>
    <html>
    <head>
    <meta content='text/html; charset=ISO-8859-2'
    http-equiv='content-type'>
    <title></title>
    </head>
    <body>
    <table>"

i = 0
people.each { |person|
    array(document, person, i)
    i += 1
}

document << 
    "</table>
    </body>
    </html>"

File.open('document.html', 'w:ISO-8859-2') { |f| f.write document }
\$\endgroup\$

1 Answer 1

Reset to default
2
\$\begingroup\$

Your code is basically a long a list of discrete steps, rather than an attempt at modelling the data and the task.

The steps for both output formats are

  1. Read
  2. Parse
  3. Write

I think your teacher would want you to realize that the first 2 steps are completely independent of the output format: Regardless of whether it's HTML or RTF (or JSON or whatever), you have to read and parse an input file. So get those 2 steps working first, and reuse the same code (e.g. require 'parser') for either output format.

There's also a lot of built-in stuff in Ruby that you could use to your advantage. So read the docs. Read as much as you can stomach and then some. Know your tools.
In this case, this applies especially to the docs for Enumerable/Array and IO/File, which are things you're most likely to use for this and many, many other things (Enumerable especially)

Other things in your code:

  • Don't just include the entire RTF module in the global scope; put your own code in the RTF module's namespace, include it in a class you define, or just live with writing RTF:: in a couple of places. Any of those things are preferable.

  • Don't call a method array. What is one supposed to think when reading that name? It's apparently the method that does everything yet it's just named "array", which is a data type/object/concept - everything but a method. Name your methods for what they do or what they are (i.e. their return value). So: Don't name a method card, since it isn't a card; the method does something but that something is not "card" either. Same with the b method, which also doesn't really reveal what it does.

  • Why are you reading a file as UTF-8, yet writing it as iso-8859-2 when writing the HTML?

  • You're not really outputting tables. You're just writing a lot of paragraphs, which you can do by, well, just writing a bunch of paragraphs. No table needed. But I imagine that you're supposed to output an actual table with a header row, and the data for each person as a row in that table.

  • Magic, hard coded numbers: No thanks. E.g. width 4000 for table cells? Why 4000?

To answer some of you comments:

I could bet that there's some way to automatically apply line_break command after all following lines.

Yes: A method. Code something like write_line(text), which writes some text to the document followed by a line break.

Methods don't need to be giant blocks of code. In fact, the shorter a method is, the better. So a 2-line method to write something plus a line break (which also matches the method's name) is a great way to go.

I don't like using a global variables there but applying them directly in method card would slow down whole script.

Would it? Because I doubt it. But if you're sure: How much does it slow down? And: Does it matter? (My guess here is "very, very little", and "no, it doesn't matter at all")

I don't like using "i" variable as cell counter of table. I would like to use some useful method instead of that.

See Enumerable#each_with_index

Anyway, code: I'd start by defining a parser class:

class Parser
  # cards is an array of hashes
  attr_reader :cards

  # Constructor - takes a file path as its argument
  def initialize(path)
    # Read the lines, and parse them individually
    # in a private method. The method may return nils,
    # so call `compact` to get rid of those
    @cards = File.foreach(path).map(&method(:parse_line)).compact
  end

  # This returns the names of attributes of a card, i.e.
  # the individual pieces of information for a person.
  # This could be a constant, but I personally prefer
  # a method for this so it can be overridden in sub-
  # classes if necessary
  def attributes
    @attributes ||= %w(name surname date_of_birth salary phone position email).freeze
  end

  private

  # parse a line from the input file and return a hash
  # or nil, if the line's empty
  def parse_line(line)
    # grab the first n pieces of data in the line
    data = line.strip.split(/;/)[0, attributes.count]
    if data.any?
      Hash[ attributes.zip(data) ] # create a hash
    end
  end
end

That's all you need to read the input file, and convert it into hashes that make the task of outputting the data easier:

Parser.new("base.txt").cards #=> [{ "name": "foo", "surname": "bar", "date_of_birth": "1/1/1970", "salary": "123456", "phone": "1234567", "position": "whatever", "email": "[email protected]" }, ... etc ]

For writing, I'd recommend making 2 classes: HtmlWriter and RtfWriter. Both operate the same way: They take a parser object and use its cards array to output the information.

Here's a (somewhat overkill) example of a HTML writer class. I'm "cheating" and using CSS to do the alternating row colors.

class HtmlWriter
  attr_reader :parser

  def initialize(parser)
    @parser = parser
  end

  def write(file_path)
    File.write(file_path, content)
  end

  def content
    <<-HTML
<!DOCTYPE html>
<head>
  <meta charset="utf-8">
  <title></title>
  <style>
    tbody tr { background-color: maroon; color: olive }
    tbody tr:nth-child(odd) { background-color: olive; color: maroon }
  </style>
</head>
<body>
  #{table}
</body>
</html>
    HTML
  end

  protected

  def table
    html_tag('table', "#{table_header}\n#{table_body}")
  end

  def table_header
    html_tag('thead') do
      html_tag('tr') do
        parser.attributes.map { |value| html_tag('th', value) }
      end
    end
  end

  def table_body
    html_tag('tbody') do
      parser.cards.map do |card|
        html_tag('tr') do
          card.map { |_, value| html_tag('td', value) }
        end
      end
    end
  end

  def html_tag(name, content = nil)
    content = yield if block_given?
    content = content.join if content.respond_to?(:join)
    "<#{name}>#{content}</#{name}>"
  end
end

Or for RTF (I'm sure this could be be improved - I don't know the RTF gem well)

class RtfWriter
  include RTF

  attr_reader :parser

  def initialize(parser)
    @parser = parser
  end

  def write(path)
    File.write(path, document.to_rtf)
  end

  def document
    @document ||= begin
      doc = Document.new(Font.new(Font::ROMAN, 'Times New Roman'))
      table = doc.table(parser.cards.count + 1, parser.attributes.count)
      add_header_row(table)
      add_body_rows(table)
      parser.cards.count.times do |row|
        table.row_shading_color(row + 1, color_for_row(row))
      end
      doc
    end
  end

  private

  def add_header_row(table)
    parser.attributes.each_with_index.map do |name, column|
      table[0][column].bold { |node| node << name }
    end
  end

  def add_body_rows(table)
    parser.cards.each_with_index.map do |card, row|
      card.values.each_with_index do |value, column|
        table[row + 1][column] << value
      end
    end
  end

  def color_for_row(row)
    colors[row % colors.count]
  end

  def colors
    @colors ||= [
      Colour.new(125, 0, 0),
      Colour.new(125, 125, 0)
    ]
  end
end

And, just for fun, here's a JSON formatter...

require 'json/ext' # or just 'json'

File.write("cards.json", Parser.new('base.txt').cards.to_json())
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.