I don't like my code -- what can I do?

[Daniel Waite]

(Disclaimer: I'm using Rails, but my issue isn't with Rails. I also find
the quality of posts on the Ruby forum of slightly higher quality more
frequently than the Rails side. Also, this is a long post...)

I'm programming a bidding system, and I don't like my code. I can
clearly see where it's wrong, and even explain why, but I don't know how
to make it better.

In a nutshell...

I have Part objects (billable things like envelopes), Vendor objects
(suppliers of said parts) and BidRequest objects (a request by the
client (me) to a specific vendor for prices regarding a particular part
-- includes actual prices, too (not my choice)).

Now... the way our interface is laid out, users spend a lot of time
looking at parts. So when they want to request a bid from a vendor, they
do so from the viewpoint of a part. "I want a bid for this part from
these vendors." They check off the vendors they want to get bids from,
the program creates BidRequest objects whose status is OPEN and an
e-mail gets sent off.

When the vendor gets the e-mail there's a link to bid on the part. The
vendor is taken to a form to put in prices, and upon submission, the bid
request is completed and its status set to COMPLETE. The vendor can also
decline, in which case the status is set to DECLINED. There's also
CANCELED, EXPIRED and DEFAULT statuses, too.

CANCELED and EXPIRED are easy enough. It's DEFAULT that is (I think) the
source of the stench. Here's why:

# This method accepts vendor pricing and changes
# the status to COMPLETE.
bid_request.bid_with(options)

# This method is meant for us (the company) to
# setup default pricing for a vendor. The default
# prices are stored internally as a BidRequest
# with a status of DEFAULT. They are inaccessible
# to the vendor.
part.set_default_prices_for_vendor(options) # options includes vendor_id

# I started to write this method when I realized I
# already have that functionality with part.
bid_request.bid_with_as_default(options)

Gah, this is really difficult to explain.

Basically what it comes down to is that the
part#set_default_prices_for_vendor needs to set the status of the bid
request -- thus violating encapsulation of the BidRequest. I believe
that only the BidRequest should know anything about possible statuses.
The code looks something like this...

def set_default_prices_for_vendor(options)
bid_requests.create(options.merge({ :status => 'DEFAULT' })
end

I've since removed that method from Part and wrote...

def bid_with_as_default(options)
ensure_expires_attribute_is_a_date(options)
update_attributes(options.merge({ :status => 'DEFAULT' })) ? true :
false
end

But this makes my controller code ugly (I think) because of the way the
interface is setup. The fact that it's heavily biased towards parts and
not vendors leaves me wanting to write things like
part.request_bid_from_vendor and part.set_default_prices_for_vendor but
then Part knows too much about BidRequest.

If it were biased towards vendors I imagine I'd write the inverse of
what I just showed you. But is there a way to be biased towards neither?
Take a cue from REST and treat BidRequest like a resource?

I often think of things as stories and try to write objects to fulfill
the roles in those stories, but my results are ugly and smell bad. So
I'm left wondering...

1. Is it a design decision?
Does my method of programming not work as well as it could because the
overarching design doesn't allow for it?

2. Can meta-programming or functional programming help?
I don't know enough about the how, why and when to even begin with this,
but since I didn't write the spec (I hate that term) I'm kind of stuck.
(Kind of -- if I could write a kick ass replacement that was super clean
I'm sure they'd go for it.)

3. Is there a pattern that someone sees that I don't?
I suppose it would help to be familiar with more than a few patterns
(I'm familiar with MVC and observer only because of Rails, I've read up
on the decorator and factory patterns -- so I've no street smarts, only
what I've read and can remember.)

4. Is the problem domain inherently difficult?

So... you've made it this far! I applaud and appreciate your effort.
Now, do you have any advice? Hehe... =)

Thank you very, very much!

 

[John Joyce]

design problem.

you have:

customers
vendors
bids
parts
request for bids
orders


Just think about your relationships again here.
Think like you might think with ActiveRecord associations. (in fact
that's probably what you need here)
You're trying to do too much with one model. (that's more like C
structs)
You need more models that can interact with each other. (more like
Ruby objects)
You may need to break it down further...

But just make your little sketches on paper.
There are a few perspectives you could take here, but start with the
most basic items
You may need to alter the naming. Sometimes it can be confusing.
is a bid the price offered by a vendor?
or is a bid the whole response from the vendor?

One thing to be wary of are words that are verbs and nouns.
objects tend to be nouns and methods tend to be verbs or predicates.

 

[Daniel Waite]

design problem.

That's what I thought. The specs are being written by someone who is
fluent in Java (I believe) and he puts a heavy emphasis on SQL. Granted,
he's forgiven to a great extent as the program we are porting isn't of
his own design. He's said before he feels it's a poor design, too. But
still... SQL everywhere (not in the views though... thankfully)!

You're trying to do too much with one model. (that's more like C
structs) You need more models that can interact with each other.
(more like Ruby objects) You may need to break it down further...

I definitely agree with the too much responsibility for one model. It's
perverted and I feel dirty.

But just make your little sketches on paper.
There are a few perspectives you could take here, but start with the
most basic items You may need to alter the naming. Sometimes it can
be confusing. is a bid the price offered by a vendor? or is a bid
the whole response from the vendor?

One thing to be wary of are words that are verbs and nouns.
objects tend to be nouns and methods tend to be verbs or predicates.

Yes, yes, yes! You read my mind! (I'm almost welling up!) I've said this
for a long time! Words are sometimes the most difficult part of
programming! Words MEAN something. And programming isn't politics, so
meaning should be preserved as much as possible -- what do I call the
adapter part of a polymorphic association to an appointment?
"Schedulable" isn't a word, so it doesn't make sense to say that (one
example I recently came across -- I settled on available).

Thanks John, you've made my day. Whew... time to get back to work!

 

[John Joyce]

That's what I thought. The specs are being written by someone who is
fluent in Java (I believe) and he puts a heavy emphasis on SQL.
Granted,
he's forgiven to a great extent as the program we are porting isn't of
his own design. He's said before he feels it's a poor design, too. But
still... SQL everywhere (not in the views though... thankfully)!


I definitely agree with the too much responsibility for one model.
It's
perverted and I feel dirty.


Yes, yes, yes! You read my mind! (I'm almost welling up!) I've said
this
for a long time! Words are sometimes the most difficult part of
programming! Words MEAN something. And programming isn't politics, so
meaning should be preserved as much as possible -- what do I call the
adapter part of a polymorphic association to an appointment?
"Schedulable" isn't a word, so it doesn't make sense to say that (one
example I recently came across -- I settled on available).

Thanks John, you've made my day. Whew... time to get back to work!

No problem, I struggle with this stuff myself, and I just do it for a
hobby.
I don't think it's java getting in his way.
Nor is it SQL
It's just a matter of getting the the right data relationships.
SQL isn't inherently bad just how it's used can be bad.
You just need to create objects and let SQL do the storage and
retrieval and use it to get some of the freebies you get from it,
like cheap fast indexing and datamining, but those things should come
after the fundamental design.
Your situation is simple and should be pretty easy to develop if the
models are right.

 

[Daniel Waite]

It's just a matter of getting the the right data relationships.
SQL isn't inherently bad just how it's used can be bad.
You just need to create objects and let SQL do the storage and
retrieval and use it to get some of the freebies you get from it,
like cheap fast indexing and datamining, but those things should come
after the fundamental design.
Your situation is simple and should be pretty easy to develop if the
models are right.

Agreed.

I'm looking at it now through fresh eyes -- not thinking at all about
the spec set forth, and I'm amazed at what I'm finding.

First off, a huge amount of dependency is coming from the fact that when
created, a BidRequest generates an access key of sorts (a SHA1 hash of
some attributes) that acts as a "key" to get to the bidding screen. This
key is unique per vendor, which automatically means there can only be
one bid request per vendor, and that the bid request is literally tied
to the vendor.

I just had a meeting where I found out that the company sometimes does
promotions (or packages, more accurately, I think) whereby they request
bids on a collection of parts. Well hello!

In my mind, whenever you have more than one of something, no matter how
rare, you have a collection. Period. Forget trying to code for those
exceptions, just make it a collection.

I'm also beginning to see a behavior... the ability to be bid on -- or
biddable. Biddable things share similar attributes and behavior
regardless of what they are. Sweet!

Thanks again, John. Sometimes I need to be reminded that just because
"the boss" says do it this way doesn't mean it's the best way to be
done. (Actually, I rarely need to be reminded of that; I've been fired
from so many jobs for being not only the FIRST but the LOUDEST to say
that the proposed method of doing things is a poor choice. I think I
crawled into a shell and fell into the trap of "going along to get
along" after being unemployed for so long.)

 

[JeremyWoertink]

I think that other guy should be fired and I should be hired

 

[John Joyce]

That's all fine and well. I think it is an opportunity to make some
good code, then explain to the boss that it works well, and why. With
Ruby and/or Rails it should be a lot easier to implement.
Just remember though, that one thing that can kill a Rails
application is trying to bend it to fit an existing database too
much. Build your database to fit your application, then if you need
to translate that to an existing database, build a bridge.

 

[mortee]

But this makes my controller code ugly (I think) because of the way the
interface is setup. The fact that it's heavily biased towards parts and
not vendors leaves me wanting to write things like
part.request_bid_from_vendor and part.set_default_prices_for_vendor but
then Part knows too much about BidRequest.

If it were biased towards vendors I imagine I'd write the inverse of
what I just showed you. But is there a way to be biased towards neither?
Take a cue from REST and treat BidRequest like a resource?

Hi, just to drop in my $.02 naively. I guess that some part of your code
would have to know about BidRequest statuses anyway. It's either the
Parts which can collect corresponding requests belonging to a specific
vendor, and selecting which applies. Or the Vendor can, given a Part,
select the corresponding requests and select the appropriate one.

If you want neither to know about the ins and outs of this selection
algorithm, you'll need a BidRequest broker, which, given a Part and a
Vendor (and possibly a Client, too), yields you with whatever bid
applies out of all the available ones - taking into consideration
whether whether these are valid completed ones, and in case there's no
such bid, then look for a default one. You could then use this broker to
specify those default bids, too.

The logic has to be specified at one place or another, it's just a
question of whether you really want to decouple it from everything, or not.

mortee