Badly organized code?

Ruby
1

Hi all,

I am writing generic command line scripts and here is a minimal example:
http://pastie.org/979581 - which I am quite happy with. Now the required
‘biopieces’ class is here http://pastie.org/979577 - but this I am not
really happy with. Mainly, I think it is poorly organized? Suggestions?

Martin

2

2010/5/27 Martin H. [email protected]:

I am writing generic command line scripts and here is a minimal example:
http://pastie.org/979581 - which I am quite happy with. Now the required
‘biopieces’ class is here http://pastie.org/979577 - but this I am not
really happy with. Mainly, I think it is poorly organized? Suggestions?

I would extract the opion parsing from that class and either use a
Hash, OpenStruct or custom class for transportation of your options.
That way you make your bio pieces processing independent from the
interface (command line, graphical application etc.).

Then I would extend the parsing algorithm and offer a similar
interface like CSV or File along the lines of:

class BioPieces
include Enumerable

def self.open(file_name)
File.open file_name do |io|
yield new io
end
end

def self.foreach(file_name, &b)
open file_name do |bp|
bp.each_record(&b)
end
end

def initialize(io)
@io = io
end

def each_record
rec = {}

@io.each_line do |line|
  case line
  when /^([^:])+:\s*(.*)$/
    rec[$1.strip] = $2.strip
  when /^-+$/
    yield rec
    rec = {}
  else
    raise "Invalid input: %p" % line
  end
end

# maybe add this:
# yield rec unless rec.empty?

self

end

alias each each_record
end

Btw, your code looks pretty clean and well documented. That’s good!

Kind regards

robert

3

On Thu, May 27, 2010 at 7:26 AM, Martin H. [email protected] wrote:

Hash, OpenStruct or custom class for transportation of your options.
Thanks, I am working hard on it!
Posted via http://www.ruby-forum.com/.

If you have a comprehensive set of tests, then people can refactor
without
fear of breaking.

4

Hi Robert,

It is quite important to me that other people can put together a
“Biopiece” as easily as possible i.e. expand the minimal example.
Therefore I have moved code which under normal circumstances would be
placed in the ‘Biopieces’ script to the Biopieces class - and I quite
possibly violate a number of conventions that way.

I would extract the opion parsing from that class and either use a
Hash, OpenStruct or custom class for transportation of your options.
That way you make your bio pieces processing independent from the
interface (command line, graphical application etc.).

I really don’t want upcoming ‘Biopiece writers’ to fiddle around with
other classes than the Biopieces class for the sake of simplicity - and
I am willing to have option parsing and record parsing mixed even though
it is ugly.

Btw, your code looks pretty clean and well documented. That’s good!

Thanks, I am working hard on it!

I was wondering if nested classes might bring some order to this?

Cheers,

Martin

5

2010/5/27 Martin H. [email protected]:

I really don’t want upcoming ‘Biopiece writers’ to fiddle around with
other classes than the Biopieces class for the sake of simplicity - and
I am willing to have option parsing and record parsing mixed even though
it is ugly.

Well, then I don’t know what you’re after. You say you suspect it’s
badly structured, someone points out something that indicates bad
structure and you say you want to keep it that way. What is your goal
and what do you expect from us?

Cheers

robert

6

The goal is to create a bunch of scripts that can be piped together
easily on the command line in a UNIX environment to manipulate text
based records. They scripts are supposed to be easy to use, but also
easy to write - in whatever programming language. Have a look here:
www.biopieces.org.

I expect/hope to learn Ruby with your fine assistance, and I want to
keep in mind good practices etc. However, for this particular project it
is difficult.

Cheers,

Martin

Robert K. wrote:

2010/5/27 Martin H. [email protected]:

I really don’t want upcoming ‘Biopiece writers’ to fiddle around with
other classes than the Biopieces class for the sake of simplicity - and
I am willing to have option parsing and record parsing mixed even though
it is ugly.

Well, then I don’t know what you’re after. You say you suspect it’s
badly structured, someone points out something that indicates bad
structure and you say you want to keep it that way. What is your goal
and what do you expect from us?

Cheers

robert

7

On Thu, May 27, 2010 at 8:05 AM, Martin H. [email protected] wrote:

fear of breaking.

I meant rubyists. For example, the time_diff method at the very end can
probably be rewritten

def time_diff(t0, t1)
day0 , hour0 , min0 , sec0 = t0.split(/\D+/)[2…-1].map { |n| n.to_i }
day1 , hour1 , min1 , sec1 = t1.split(/\D+/)[2…-1].map { |n| n.to_i }

sec0 += ( ( day0 * 24 + hour0 ) * 60 + min0 ) * 60
sec1 += ( ( day1 * 24 + hour1 ) * 60 + min1 ) * 60
sec = sec1-sec0

sprintf ‘%02d:%02d:%02d’ , sec/60/60 , sec/60%60 , sec%60
end

But I don’t know for sure, without a bunch of tests to show it behaves
the
same way. I think the math at the end is probably right, but haven’t had
a
math course in 2 years, would you rather trust correctness to my
“probably
right” or to your ironclad test suite? And when looking at it, I see we
don’t use year or month, so I wonder if it has bugs, and I’m just
refactoring bugs, which makes it very difficult to get motivated. I
might
also be inclined to play with DateTime to see if it has this
functionality
within it, in which case the whole method could probably be replaced
with 1
line, but its not immediately obvious to me if it does, so it would be
nice
to have some tests that I can know how close I am getting.

8

Josh C. wrote:

On Thu, May 27, 2010 at 7:26 AM, Martin H. [email protected] wrote:

Hash, OpenStruct or custom class for transportation of your options.
Thanks, I am working hard on it!
Posted via http://www.ruby-forum.com/.

If you have a comprehensive set of tests, then people can refactor
without
fear of breaking.

I am working on comprehensive unit testing of class biopieces, but
“Biopiece writers” should not change anything in that class. They should
only concentrate on what is in the script they are writing and whatever
classes they write to expand that.

Martin

9

@Josh,

Sure, I actually want to use some of the time math ruby provides, and
write appropriate tests for it. I just used some old crap as a place
holder for now. I am in the process of writing tests and when I get to
it, I will most certainly refactor.

;o)

M

Josh C. wrote:

On Thu, May 27, 2010 at 8:05 AM, Martin H. [email protected] wrote:

fear of breaking.

I meant rubyists. For example, the time_diff method at the very end can
probably be rewritten

def time_diff(t0, t1)
day0 , hour0 , min0 , sec0 = t0.split(/\D+/)[2…-1].map { |n| n.to_i }
day1 , hour1 , min1 , sec1 = t1.split(/\D+/)[2…-1].map { |n| n.to_i }

sec0 += ( ( day0 * 24 + hour0 ) * 60 + min0 ) * 60
sec1 += ( ( day1 * 24 + hour1 ) * 60 + min1 ) * 60
sec = sec1-sec0

sprintf ‘%02d:%02d:%02d’ , sec/60/60 , sec/60%60 , sec%60
end

But I don’t know for sure, without a bunch of tests to show it behaves
the
same way. I think the math at the end is probably right, but haven’t had
a
math course in 2 years, would you rather trust correctness to my
“probably
right” or to your ironclad test suite? And when looking at it, I see we
don’t use year or month, so I wonder if it has bugs, and I’m just
refactoring bugs, which makes it very difficult to get motivated. I
might
also be inclined to play with DateTime to see if it has this
functionality
within it, in which case the whole method could probably be replaced
with 1
line, but its not immediately obvious to me if it does, so it would be
nice
to have some tests that I can know how close I am getting.