Badly organized code?

[Martin Hansen]

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

 

[Robert Klemme]

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

 

[Martin Hansen]

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

 

[Josh Cheek]

[Note: parts of this message were removed to make it a legal post.]

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 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.


Thanks, I am working hard on it!


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


Cheers,


Martin

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

 

[Martin Hansen]

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

 

[Robert Klemme]

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 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

 

[Martin Hansen]

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

 

[Josh Cheek]

[Note: parts of this message were removed to make it a legal post.]

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

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.

 

[Martin Hansen]

@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

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.