# Badly organized code?

**URL:** <https://www.ruby-forum.com/t/badly-organized-code/189239>\
**Category:** Ruby\
**Created:** [May 27, 2010, 11:50am UTC](https://www.ruby-forum.com/t/badly-organized-code/189239 "2010-05-27T11:50:05Z")\
**Posts on this page:** 9\
**Page:** 1

<div class="post-metadata">

**Author:** ![himdel](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/h/c67d28/32.png) [@himdel](https://www.ruby-forum.com/u/himdel)\
**Post date:** [May 27, 2010, 11:50am UTC](https://www.ruby-forum.com/t/badly-organized-code/189239/1 "2010-05-27T11:50:05Z")

</div>

Hi all,

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

Martin

---

<div class="post-metadata">

**Author:** ![himdel](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/h/c67d28/32.png) [@himdel](https://www.ruby-forum.com/u/himdel)\
**Post date:** [May 27, 2010, 1:53pm UTC](https://www.ruby-forum.com/t/badly-organized-code/189239/2 "2010-05-27T13:53:13Z")

</div>

2010/5/27 Martin H. [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid):

> I am writing generic command line scripts and here is a minimal example:  
> [http://pastie.org/979581](http://pastie.org/979581) - which I am quite happy with. Now the required  
> ‘biopieces’ class is here [http://pastie.org/979577](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

---

<div class="post-metadata">

**Author:** ![himdel](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/h/c67d28/32.png) [@himdel](https://www.ruby-forum.com/u/himdel)\
**Post date:** [May 27, 2010, 2:48pm UTC](https://www.ruby-forum.com/t/badly-organized-code/189239/3 "2010-05-27T14:48:15Z")

</div>

On Thu, May 27, 2010 at 7:26 AM, Martin H. [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid) 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/](http://www.ruby-forum.com/).

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

---

<div class="post-metadata">

**Author:** ![himdel](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/h/c67d28/32.png) [@himdel](https://www.ruby-forum.com/u/himdel)\
**Post date:** [May 27, 2010, 2:26pm UTC](https://www.ruby-forum.com/t/badly-organized-code/189239/4 "2010-05-27T14:26:34Z")

</div>

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

---

<div class="post-metadata">

**Author:** ![himdel](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/h/c67d28/32.png) [@himdel](https://www.ruby-forum.com/u/himdel)\
**Post date:** [May 27, 2010, 3:10pm UTC](https://www.ruby-forum.com/t/badly-organized-code/189239/5 "2010-05-27T15:10:29Z")

</div>

2010/5/27 Martin H. [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid):

> 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

---

<div class="post-metadata">

**Author:** ![himdel](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/h/c67d28/32.png) [@himdel](https://www.ruby-forum.com/u/himdel)\
**Post date:** [May 27, 2010, 4:19pm UTC](https://www.ruby-forum.com/t/badly-organized-code/189239/6 "2010-05-27T16:19:59Z")

</div>

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](http://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. [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid):
> 
> > 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

---

<div class="post-metadata">

**Author:** ![himdel](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/h/c67d28/32.png) [@himdel](https://www.ruby-forum.com/u/himdel)\
**Post date:** [May 27, 2010, 4:37pm UTC](https://www.ruby-forum.com/t/badly-organized-code/189239/7 "2010-05-27T16:37:43Z")

</div>

On Thu, May 27, 2010 at 8:05 AM, Martin H. [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid) 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.

---

<div class="post-metadata">

**Author:** ![himdel](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/h/c67d28/32.png) [@himdel](https://www.ruby-forum.com/u/himdel)\
**Post date:** [May 27, 2010, 3:05pm UTC](https://www.ruby-forum.com/t/badly-organized-code/189239/8 "2010-05-27T15:05:56Z")

</div>

Josh C. wrote:

> On Thu, May 27, 2010 at 7:26 AM, Martin H. [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid) 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/](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

---

<div class="post-metadata">

**Author:** ![himdel](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/h/c67d28/32.png) [@himdel](https://www.ruby-forum.com/u/himdel)\
**Post date:** [May 27, 2010, 5:04pm UTC](https://www.ruby-forum.com/t/badly-organized-code/189239/9 "2010-05-27T17:04:23Z")

</div>

@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. [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid) 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.