On Fri, Mar 19, 2010 at 10:31 AM, Robert K.
[email protected]wrote:
which did not (yet) make it into the mailing list.
Kind regards
robert
–
remember.guy do |as, often| as.you_can - without end
http://blog.rubybestpractices.com/
Hi, Robert
I don’t have the time to go completely through your code. Few remarks
anyway:
That is fine, I appreciate what you did take the time to go through
- It is not clear to me why you have MailRecord composed of Records.
I’d probably rather have picked another name, e.g. MailFile. Class
comments would also help.
That name would probably make sense. I’m not sure what you mean by Class
comments, though.
- Your method #validate is invoked on individual fields but you rather
want to check the complete record length.
Well, each line must match the line length, or when we try to pull
specific
attributes from that record they will not be correct. If each line is
the
correct length, then the record is the correct length.
- You can use Struct to easily define Record in less lines
Record = Struct.new :title , :id , :from , :to , :offset
That makes a lot of sense, thanks for the tip, I default to OO first,
because that is what I am most familiar with, so structs don’t come
quickly
to mind. Though I have read your article probably three times
http://blog.rubybestpractices.com/posts/rklemme/017-Struct.html it
usually
only comes to mind it when my structure is very dynamic, ie OpenStruct
- Your MailRecord is a good abstraction of the file storage.
Thank you
- I would not store the position of the record in the Record instance
because that way you mix business logic state (record contents) and
storage related state. If you have another storage medium your position
in the Record will be superfluous.
- I would not use puts with your fixed size records. Rather I would use
write and read. Plus, I’d place those methods in MailRecord and not in
Record because they are specific to this particular storage medium.
I understand your points here, but I am having difficulty thinking of a
way
to implement this. Perhaps the record should have a reference to the
MailRecord (or MailFile, as you suggest), and then tell the file to
write
itself? But the position thing still seems to be an issue.
Maybe the problem is that I am considering it to be almost an array of
files
based on position in the file, but I should remove index/offset from
consideration and instead consider it as a set, where ordering is
arbitrary
and can be altered as necessary to accommodate encapsulated logic and
data
integrity.
I’m not really sure what a proper approach would look like here.
Anyway, thanks for taking the time to look and comment