-1
\$\begingroup\$

I'm on chapter 7 of Chris Pine's book, trying to work on the last exercise in that chapter. This code works fine, but I'm wondering if there is a better way to do this...

toc_array = ['Table of Contents',  'Chapter 1:  Numbers', 'page 1', 'Chapter 2:  Letters',  'page 72', 'Chapter 3:  Variables', 'page 118']
linewidth = 30
puts toc_array[0].center(linewidth*2)
puts (toc_array[1].ljust(linewidth)) + (toc_array[1].rjust(linewidth))
puts (toc_array[2].ljust(linewidth)) + (toc_array[3].rjust(linewidth))
puts (toc_array[4].ljust(linewidth)) + (toc_array[5].rjust(linewidth))

Is there more efficient way to print the formatted array?

\$\endgroup\$
  • 2
    \$\begingroup\$ What does this have to do with sorting (as indicated in the title)? \$\endgroup\$ – 200_success Apr 2 '14 at 19:13
  • \$\begingroup\$ Elements of the array have to be sorted [0-5] and then formatted in a certain way. \$\endgroup\$ – user39963 Apr 2 '14 at 19:32
  • \$\begingroup\$ They are sorted, as far as I can tell. Heading, followed by chapters and page numbers in their proper order. All that's left is formatting \$\endgroup\$ – Flambino Apr 2 '14 at 19:36
  • \$\begingroup\$ Please check that you got your array indexes right. The output looks suspiciously wrong. \$\endgroup\$ – 200_success Apr 2 '14 at 19:44
  • \$\begingroup\$ There is no output in my message. If you do run it, it will print fine. Thanks for your reply. \$\endgroup\$ – user39963 Apr 2 '14 at 20:01
3
\$\begingroup\$

Er, no, the code doesn't work fine. I get:

                     Table of Contents                      
Chapter 1:  Numbers                      Chapter 1:  Numbers
page 1                                   Chapter 2:  Letters
page 72                                Chapter 3:  Variables

Chapters and page numbers are reversed (compared to a regular table of contents anyway), and chapter 1's page number isn't shown.

Anyway, I assume you can figure out what array indices should be fixed.

Point #2: linewidth is a bad name. It's exactly half of the desired linewidth.

Point #3: DRY - Don't Repeat Yourself. Go read the documentation for Array and the Enumerable module that's included in Array: You practically never have to manually copy lines and change indexes

Given these inputs

toc_array = ['Table of Contents',  'Chapter 1:  Numbers', 'page 1', 'Chapter 2:  Letters',  'page 72', 'Chapter 3:  Variables', 'page 118']
line_width = 60
half_line_width = line_width / 2

it's much easier to do this

puts toc_array.shift.center(line_width)
toc_array.each_slice(2) do |chapter, page|
  puts chapter.ljust(half_line_width) + page.rjust(half_line_width)
end

which gives you

                     Table of Contents                      
Chapter 1:  Numbers                                   page 1
Chapter 2:  Letters                                  page 72
Chapter 3:  Variables                               page 118
\$\endgroup\$
  • \$\begingroup\$ Ooo... there are many things there that I don't know yet. What are those (2) arguments in the array toc_array.each_slice(2)? \$\endgroup\$ – user39963 Apr 2 '14 at 20:18
  • \$\begingroup\$ @user39963 See the the docs for each_slice (already linked in my answer). If you mean the two arguments for the block: Destructuring. each_slice(2) loops over an array, 2 elements at a time, passing those elements to the block as a array. But because ruby let's you use destructuring, the two elements are assigned to chapter and title respectively. \$\endgroup\$ – Flambino Apr 2 '14 at 20:24
3
\$\begingroup\$

Your code is very brittle and hard-coded. Code is supposed to be as generic and flexible as possible so you could re-use it. A new chapter, a different layout, anything would require you to change the whole code.

Code is about doing work for you. Think generically - can this code be used for a different book? (I don't mean by you copying it, pasting it and changing it...)

You need to abstract it - what do you have here?

You have a title, which is different than a chapter. A chapter has a page number. There are some chapters in a book. The TOC needs to be laid out using some logic...

title and chapter are objects is your system, the layout logic is a method.

For example - I pass to a method called format_toc a title, and a list of chapters, which (for simplicity) contain their name and their page number in an array. The method than lays out the TOC page according to the format in your code. Note that the data does not contain the words 'Chapter' or 'page' - they are part of the layout, not the data:

def format_toc(title, chapters)
  line_width = 30
  toc = chapters.each_with_index.map do |chapter, i|
    "Chapter #{i}: #{chapter[0]}".ljust(line_width) + "page #{chapter[1]}".rjust(line_width)
  end.join("\n")
  "#{title.center(line_width*2)}\n#{toc}"
end
puts format_toc 'Table of Contents', [['Numbers', 1], ['Letters',  72], ['Variables', 118]]
#=>                     Table of Contents                      
#=>Chapter 1:  Numbers                                   page 1
#=>Chapter 2:  Letters                                  page 72
#=>Chapter 3:  Variables                               page 118

But wait - you are still using hard-coded indexes!
True, [0] and [1] are still there, which looks a little "hard-coded", but these indexes refer to the data structure, not the data itself. A more elaborate design would likely use a hash to hold chapter data ({name: 'Numbers', page: 1}), or even a class having two properties, but for the example above it shows the concept of encapsulation - as long as two modules know the language (=API), they can independently change things in their domain (wither adding chapters to the book, or change the layout), without affecting other parts of the code.

I also took the liberty of changing the structure of the input, as @Flambino remarked, but only because I assumed the choice of the structure of the input was not an outside constraint, but a design choice (it didn't make much sense otherwise).

\$\endgroup\$
  • \$\begingroup\$ Sorry, but -1 for the code: You still have a hard coded index, and you're changing the input array's "format" to suit your code \$\endgroup\$ – Flambino Apr 2 '14 at 20:16
  • \$\begingroup\$ Thank you, Uri. I liked the main concepts you mentioned in your answer. It never occurred to me to make layout a method. \$\endgroup\$ – user39963 Apr 2 '14 at 20:29
  • \$\begingroup\$ @Flambino - see my addition to the answer \$\endgroup\$ – Uri Agassi Apr 3 '14 at 6:15
  • \$\begingroup\$ Read it, and must admit I was too quick on the trigger earlier - I apologize, and I've cancelled my downvote. For whatever reason, I read chapter[i] in one place, and chapter[1] in another, and assumed it was leftover code - that's what I meant by "hard coded indexes", but I simply misread it. As for changing the input data's structure, I assumed the opposite of you: that it was an outside constraint. I don't know. But I figured that was "the challenge" so to speak; otherwise the simplest solution is to structure the input to match the intended output and skip the code :) \$\endgroup\$ – Flambino Apr 3 '14 at 6:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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