Code Review Request -- BookBot

ruby-talk
#1

Hey all,

I am trying to move my Ruby skills from "hobbyist" to "professional".
Would you mind looking at some code and answering one question?

Code:

Question:
For professional level code, what are the top three things
this code needs?

Thank you.

Leam

#2

Not really anything wrong with the code you present. Minor issues could be
that the methods used in the initialize on BookBot::Section are all public.
Do you really expect the slurp_file and ascii_scrub to be called on an
instance at any point after the object is created?

Documentation. Even if it is just a few sentences as to what the class does
and how it should be used would help. Otherwise the programmer using this
class is going to see file_name_label and ask "when should I call this?"

Not much but I expect to need to call the public methods and thus I need to
understand what they do and when I should call them. Documentation and
setting the unnecessary methods to private will make things clearer

TBH "professional" code can look a lot worse :slight_smile:

···

On Sun, 19 Jul 2020 at 16:20, Leam Hall <leamhall@gmail.com> wrote:

Hey all,

I am trying to move my Ruby skills from "hobbyist" to "professional".
Would you mind looking at some code and answering one question?

Code:
https://github.com/LeamHall/bookbot

Question:
For professional level code, what are the top three things
this code needs?

Thank you.

Leam

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

#3

To follow up on this with a specific case. The report_scrub_string takes a
parameter, which sort of shows that it is really a public method, and
updates instance variables @clean_array and @clean_string

Is a programmer really going to call this on an instance of a class? What
would happen if they did?

If not, and I suspect this is the case, then it does not need an argument,
which is an instance variable - @file_string, and should just reference it
from within the method

#4

Thanks!

You're right about the documentation. I've looked at doing ri docs but haven't kept up with it. Haven't really thought about public vs private methods much. Your explanation makes it clear why I need to.

Leam

···

On 7/19/20 1:24 PM, Peter Hickman wrote:

Not really anything wrong with the code you present. Minor issues could be that the methods used in the initialize on BookBot::Section are all public. Do you really expect the slurp_file and ascii_scrub to be called on an instance at any point after the object is created?

Documentation. Even if it is just a few sentences as to what the class does and how it should be used would help. Otherwise the programmer using this class is going to see file_name_label and ask "when should I call this?"

Not much but I expect to need to call the public methods and thus I need to understand what they do and when I should call them. Documentation and setting the unnecessary methods to private will make things clearer

TBH "professional" code can look a lot worse :slight_smile:

On Sun, 19 Jul 2020 at 16:20, Leam Hall <leamhall@gmail.com > <mailto:leamhall@gmail.com>> wrote:

    Hey all,

    I am trying to move my Ruby skills from "hobbyist" to "professional".
    Would you mind looking at some code and answering one question?

    Code:
    https://github.com/LeamHall/bookbot

    Question:
    For professional level code, what are the top three things
    this code needs?

    Thank you.

    Leam

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

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

#5

For me the top thing is how in section.rb you have a bunch of methods that
communicate by creating and modifying instance variables, and your code
relies on calling them in the correct order. I would write it more like

def initialize(file)
  raw_text = slurp_file(file)
  text = ascii_scrub(raw_text)
  clean_array = report_scrub_string(text)
  @word_count = clean_array.length
  @sentence_count = count_sentences(text)
  @syllable_count = count_syllables(clean_array)
end

martin

···

On Sun, Jul 19, 2020 at 8:20 AM Leam Hall <leamhall@gmail.com> wrote:

Hey all,

I am trying to move my Ruby skills from "hobbyist" to "professional".
Would you mind looking at some code and answering one question?

Code:
https://github.com/LeamHall/bookbot

Question:
For professional level code, what are the top three things
this code needs?

Thank you.

Leam

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

#6

A few thoughts, and I don't claim to be a "professional level" Ruby
developer, but I do use it actively in my job.


It strikes me that this clause is a bit "wet" as the saying goes. If you
could define a list of regex patterns in an Array, then iterate over the
array with gsub in one line, you could avoid calling gsub so many times in
sequence which to me, looks ugly.
Depending on what you're trying to do here, you might use an alias:

Also, I would suggest finding an opensource project which strikes your
fancy, and start picking up issues and bugs and making pull requests.
You'll get peer reviewed and if it's a good project you'll get plenty of
professional level feedback. One project I know is looking for help is Site
Prism: https://github.com/natritmeyer/site_prism or, if you are looking for
something extra extra cool, not Ruby but very interesting:
https://github.com/LMMS/lmms . LMMS is C++ code, and Ruby is C++, so great
opportunity there.

Good luck and thanks for spreading the Ruby :slight_smile:

···

On Sun, Jul 19, 2020 at 12:40 PM Martin DeMello <martindemello@gmail.com> wrote:

For me the top thing is how in section.rb you have a bunch of methods that
communicate by creating and modifying instance variables, and your code
relies on calling them in the correct order. I would write it more like

def initialize(file)
  raw_text = slurp_file(file)
  text = ascii_scrub(raw_text)
  clean_array = report_scrub_string(text)
  @word_count = clean_array.length
  @sentence_count = count_sentences(text)
  @syllable_count = count_syllables(clean_array)
end

martin

On Sun, Jul 19, 2020 at 8:20 AM Leam Hall <leamhall@gmail.com> wrote:

Hey all,

I am trying to move my Ruby skills from "hobbyist" to "professional".
Would you mind looking at some code and answering one question?

Code:
https://github.com/LeamHall/bookbot

Question:
For professional level code, what are the top three things
this code needs?

Thank you.

Leam

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

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

--
A musician must make music, an artist must paint, a poet must write, if he
is to be ultimately at peace with himself.
- Abraham Maslow

#7

Quoting Sean Felipe Wolfe (ether.joe@gmail.com):

   LMMS is C++ code, and Ruby
   is C++, so great opportunity there.

<nitpicking mode on>
Luckily, Ruby is written in C, not C++.
<nitpicking mode off>

Carlo

···

Subject: Re: Code Review Request -- BookBot
  Date: Sun 19 Jul 20 10:07:35PM -0700

--
  * Se la Strada e la sua Virtu' non fossero state messe da parte,
* K * Carlo E. Prelz - fluido@fluido.as che bisogno ci sarebbe
  * di parlare tanto di amore e di rettitudine? (Chuang-Tzu)

#8

Practically speaking I think my two biggest issues are:

1. The lack of a clear public stable API for the class
2. Referencing instance variables everywhere and creating hard dependencies
on the order in which they're set.

Both of these issues were already mentioned by others, but as an add-on I
can highly recommend reading the book Practical Object Oriented Design in
Ruby by Sandi Metz if you want to really understand *why* they're problems
and how to improve it in the future.

#9

Hey,

Tbh, I can see, essentially, only one 100+ lines class Section and two huge scripts
under bin/

From a professional I would demand to have more abstractions over the code.

For example, method `ascii_scrub` doesnt seam to logically belong to a class "section" representing
a section of a book.

As a rule of thumb, when you are inspecting a class ask yourself "is it actually its job?".

Also another interesting thing every programmer does is dividing code into logical blocks with blank lines.
You did a lot of that in your executables. When code has many logical blocks one should ask a question
"can it be isolated as a separate unit? (class)".

Bottom line, you wrote a utility software but it contains only one class.
Surely, a CLI program has more logic that one class can contain. This is an indicator
that the code is not modular enough :slight_smile:

I hope that makes sense, sorry for being a bit wordy!

···

On 19 Jul 2020, at 16:19, Leam Hall <leamhall@gmail.com> wrote:

Hey all,

I am trying to move my Ruby skills from "hobbyist" to "professional".
Would you mind looking at some code and answering one question?

Code:
https://github.com/LeamHall/bookbot

Question:
For professional level code, what are the top three things
this code needs?

Thank you.

Leam

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

#10

Hey, thanks for the feedback!

I like the "iterate over the array" idea, but need to study it to be able to do it well. Simply adding lines, like I did, is ugly. However, it is simple. :slight_smile:

The "report" method is just a start on what gets generated. It is going to include other method calls, like generating a count based word usage list. I think it may soon need a report object, but I haven't gotten that far yet.

Leam

···

On 7/20/20 1:07 AM, Sean Felipe Wolfe wrote:

A few thoughts, and I don't claim to be a "professional level" Ruby developer, but I do use it actively in my job.

https://github.com/LeamHall/bookbot/blob/master/lib/bookbot/section.rb#L42
It strikes me that this clause is a bit "wet" as the saying goes. If you could define a list of regex patterns in an Array, then iterate over the array with gsub in one line, you could avoid calling gsub so many times in sequence which to me, looks ugly.

https://github.com/LeamHall/bookbot/blob/master/lib/bookbot/section.rb#L135
Depending on what you're trying to do here, you might use an alias: https://www.rubyguides.com/2018/11/ruby-alias-keyword/

Also, I would suggest finding an opensource project which strikes your fancy, and start picking up issues and bugs and making pull requests. You'll get peer reviewed and if it's a good project you'll get plenty of professional level feedback. One project I know is looking for help is Site Prism: https://github.com/natritmeyer/site_prism or, if you are looking for something extra extra cool, not Ruby but very interesting: https://github.com/LMMS/lmms . LMMS is C++ code, and Ruby is C++, so great opportunity there.

Good luck and thanks for spreading the Ruby :slight_smile:

On Sun, Jul 19, 2020 at 12:40 PM Martin DeMello <martindemello@gmail.com > <mailto:martindemello@gmail.com>> wrote:

    For me the top thing is how in section.rb you have a bunch of
    methods that communicate by creating and modifying instance
    variables, and your code relies on calling them in the correct
    order. I would write it more like

    def initialize(file)
      raw_text = slurp_file(file)
      text = ascii_scrub(raw_text)
      clean_array = report_scrub_string(text)
      @word_count = clean_array.length
      @sentence_count = count_sentences(text)
      @syllable_count = count_syllables(clean_array)
    end

    martin

    On Sun, Jul 19, 2020 at 8:20 AM Leam Hall <leamhall@gmail.com > <mailto:leamhall@gmail.com>> wrote:

        Hey all,

        I am trying to move my Ruby skills from "hobbyist" to
        "professional".
        Would you mind looking at some code and answering one question?

        Code:
        https://github.com/LeamHall/bookbot

        Question:
        For professional level code, what are the top three things
        this code needs?

        Thank you.

        Leam

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

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

--
A musician must make music, an artist must paint, a poet must write, if he is to be ultimately at peace with himself.
- Abraham Maslow

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

#11

Ben, thanks!

Do you mind expanding a little? I really want to ensure I know what you mean. I've read Sandi's book (twice) as well as her "99 bottles" ebook. My goal is to transition from "having read the book" to "understand and confidently implement".

Leam

···

On 7/20/20 5:24 AM, Ben wrote:

Practically speaking I think my two biggest issues are:

1. The lack of a clear public stable API for the class
2. Referencing instance variables everywhere and creating hard dependencies on the order in which they're set.

Both of these issues were already mentioned by others, but as an add-on I can highly recommend reading the book Practical Object Oriented Design in Ruby by Sandi Metz if you want to really understand *why* they're problems and how to improve it in the future.

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

#12

Dmitriy, thank you! I appreciate a detailed critique. :slight_smile:

The simple answer is "yes". BookBot started as a utility script with no classes at all. This version is the first real use of a class, and I expect there to be more classes needed. I love Ruby because it seems to fit my brain more than other languages. However, I have 20+ years of habit with shell scripting; my thought process seldom starts with "objects". One of the things I'm working on is improving that.

Leam

···

On 7/20/20 6:30 AM, Dmitriy Non wrote:

Hey,

Tbh, I can see, essentially, only one 100+ lines class Section and two huge scripts
under bin/

From a professional I would demand to have more abstractions over the code.

For example, method `ascii_scrub` doesnt seam to logically belong to a class "section" representing
a section of a book.

As a rule of thumb, when you are inspecting a class ask yourself "is it actually its job?".

Also another interesting thing every programmer does is dividing code into logical blocks with blank lines.
You did a lot of that in your executables. When code has many logical blocks one should ask a question
"can it be isolated as a separate unit? (class)".

Bottom line, you wrote a utility software but it contains only one class.
Surely, a CLI program has more logic that one class can contain. This is an indicator
that the code is not modular enough :slight_smile:

I hope that makes sense, sorry for being a bit wordy!

On 19 Jul 2020, at 16:19, Leam Hall <leamhall@gmail.com> wrote:

Hey all,

I am trying to move my Ruby skills from "hobbyist" to "professional".
Would you mind looking at some code and answering one question?

Code:
https://github.com/LeamHall/bookbot

Question:
For professional level code, what are the top three things
this code needs?

Thank you.

Leam

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

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

#13

For which I am truly grateful...

···

On 7/20/20 1:55 AM, Carlo E. Prelz wrote:

  Subject: Re: Code Review Request -- BookBot
  Date: Sun 19 Jul 20 10:07:35PM -0700

Quoting Sean Felipe Wolfe (ether.joe@gmail.com):

    LMMS is C++ code, and Ruby
    is C++, so great opportunity there.

<nitpicking mode on>
Luckily, Ruby is written in C, not C++.
<nitpicking mode off>

Carlo

#14

Hey Martin, thanks!

What is the benefit from assigning the instance variable ( "@word_count = clean_array.length" ) over having the method create it? I'm still learning "Ruby conventions" and happy to learn more.

How big does initialize need to get before it is too big?

Thanks!

Leam

···

On 7/19/20 3:40 PM, Martin DeMello wrote:

For me the top thing is how in section.rb you have a bunch of methods that communicate by creating and modifying instance variables, and your code relies on calling them in the correct order. I would write it more like

def initialize(file)
raw_text = slurp_file(file)
text = ascii_scrub(raw_text)
clean_array = report_scrub_string(text)
@word_count = clean_array.length
@sentence_count = count_sentences(text)
@syllable_count = count_syllables(clean_array)
end

martin

On Sun, Jul 19, 2020 at 8:20 AM Leam Hall <leamhall@gmail.com > <mailto:leamhall@gmail.com>> wrote:

    Hey all,

    I am trying to move my Ruby skills from "hobbyist" to "professional".
    Would you mind looking at some code and answering one question?

    Code:
    https://github.com/LeamHall/bookbot

    Question:
    For professional level code, what are the top three things
    this code needs?

    Thank you.

    Leam

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

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

#15

Also, on this. I agree, but there are some projects I really need pretty soon. I write sci-fi on the side, and BookBot helps clean up the text and put everything into a readable manuscript. I'm learning LaTeX so that I can add formatting, and hopefully automate the entire "text files to Amazon ready ebook" process.

Of course, building my skills prepares me for those bigger projects, too. :slight_smile:

Leam

···

On 7/20/20 1:07 AM, Sean Felipe Wolfe wrote:

Also, I would suggest finding an opensource project which strikes your fancy, and start picking up issues and bugs and making pull requests.

#16

Getting side tracked here but isn't markdown and pandoc a suitable
workflow. Is LaTeX necessary?

This is what I used to create a little ebook for the Lapis framework (a Lua
web framework) for myself

#!/bin/sh

# --epub-cover-image=cover.jpg
# --epub-metadata=metadata.xml
# --epub-embed-font

remove_old()
{
  local FILENAME=$1

  if [ -r $FILENAME ]
  then
    echo === Removing old $FILENAME
    rm $FILENAME
  fi
}

EPUB='lapis-ref.epub'
MOBI='lapis-ref.mobi'
TEXT='text.md'

remove_old $EPUB
remove_old $MOBI
remove_old $TEXT

echo === Building $EPUB

for FILE in *.md
do
  echo --- $FILE
  cat $FILE >> $TEXT
  echo >> $TEXT
done

pandoc -o $EPUB title.yaml \
  --from=markdown+smart \
  --toc --toc-depth=2 \
  --css=epub.css \
  $TEXT

remove_old $TEXT

echo
echo === Building $MOBI

kindlegen $EPUB -o $MOBI

···

On Mon, 20 Jul 2020 at 13:10, Leam Hall <leamhall@gmail.com> wrote:

Also, on this. I agree, but there are some projects I really need pretty
soon. I write sci-fi on the side, and BookBot helps clean up the text
and put everything into a readable manuscript. I'm learning LaTeX so
that I can add formatting, and hopefully automate the entire "text files
to Amazon ready ebook" process.

Of course, building my skills prepares me for those bigger projects,
too. :slight_smile:

Leam

On 7/20/20 1:07 AM, Sean Felipe Wolfe wrote:

> Also, I would suggest finding an opensource project which strikes your
> fancy, and start picking up issues and bugs and making pull requests.

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

#17

/me reviews code for things he can steal...errr...'learn from'...

To understand things, I tend to start at a more basic level. That lets me really get into the question, and I can then write better tests. There are probably better tools for this, but I lack the understanding to feel confident with them.

···

On 7/20/20 8:31 AM, Peter Hickman wrote:

Getting side tracked here but isn't markdown and pandoc a suitable workflow. Is LaTeX necessary?

#18

Ok TBH I've never done this with regexes as I've never had a similar use
case, but in principle:

re1 = Regex.compile(/regex1/)
re2 = Regex.compile(/regex2/)
etc etc

[ re1, re2, re3, re4 ... ].each { |re| gsub(re, sub) }

Something like that. I probably have some of the syntax wrong but that's
the general idea. Leveraging the idea that you can have a regex itself as
an object, then put a series of regex objects into an array.

···

On Mon, Jul 20, 2020 at 4:44 AM Leam Hall <leamhall@gmail.com> wrote:

Hey, thanks for the feedback!

I like the "iterate over the array" idea, but need to study it to be
able to do it well. Simply adding lines, like I did, is ugly. However,
it is simple. :slight_smile:

#19

Ah this is what I had in mind. That first version seemed too verbose ...
here's a more concise example. I haven't tested it but it *should* work ??

regexes = [/r1/, /r2/, /r3/, /r4/ ]
regexes.each { |rgx| target.gsub(rgx, sub) }

Something like that.

···

On Mon, Jul 20, 2020 at 8:55 AM Sean Felipe Wolfe <ether.joe@gmail.com> wrote:

On Mon, Jul 20, 2020 at 4:44 AM Leam Hall <leamhall@gmail.com> wrote:

Hey, thanks for the feedback!

I like the "iterate over the array" idea, but need to study it to be
able to do it well. Simply adding lines, like I did, is ugly. However,
it is simple. :slight_smile:

Ok TBH I've never done this with regexes as I've never had a similar use
case, but in principle:

re1 = Regex.compile(/regex1/)
re2 = Regex.compile(/regex2/)
etc etc

[ re1, re2, re3, re4 ... ].each { |re| gsub(re, sub) }

Something like that. I probably have some of the syntax wrong but that's
the general idea. Leveraging the idea that you can have a regex itself as
an object, then put a series of regex objects into an array.

--
A musician must make music, an artist must paint, a poet must write, if he
is to be ultimately at peace with himself.
- Abraham Maslow

#20

Dmitriy,

Again, thank you for your comments. I've been working on making Book an object, messing around with sections, and realizing there are even more objects to put together.

While I still have a good bit of work to do, I wanted to let you know your comments were spot on. It's just taking me a while to refactor everything. :slight_smile:

Leam

···

On 7/20/20 6:30 AM, Dmitriy Non wrote:

Hey,

Tbh, I can see, essentially, only one 100+ lines class Section and two huge scripts
under bin/

From a professional I would demand to have more abstractions over the code.

For example, method `ascii_scrub` doesnt seam to logically belong to a class "section" representing
a section of a book.

As a rule of thumb, when you are inspecting a class ask yourself "is it actually its job?".

Also another interesting thing every programmer does is dividing code into logical blocks with blank lines.
You did a lot of that in your executables. When code has many logical blocks one should ask a question
"can it be isolated as a separate unit? (class)".

Bottom line, you wrote a utility software but it contains only one class.
Surely, a CLI program has more logic that one class can contain. This is an indicator
that the code is not modular enough :slight_smile:

I hope that makes sense, sorry for being a bit wordy!

On 19 Jul 2020, at 16:19, Leam Hall <leamhall@gmail.com> wrote:

Hey all,

I am trying to move my Ruby skills from "hobbyist" to "professional".
Would you mind looking at some code and answering one question?

Code:
https://github.com/LeamHall/bookbot

Question:
For professional level code, what are the top three things
this code needs?

Thank you.

Leam

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

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