# I don't like my code -- what can I do? **URL:** \ **Category:** Ruby\ **Created:** [October 12, 2007, 8:41pm UTC](https://www.ruby-forum.com/t/i-dont-like-my-code-what-can-i-do/117090 "2007-10-12T20:41:21Z")\ **Posts on this page:** 8\ **Page:** 1
**Author:** ![dewie](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/d/ac8455/32.png) [@dewie](https://www.ruby-forum.com/u/dewie)\ **Post date:** [October 12, 2007, 8:41pm UTC](https://www.ruby-forum.com/t/i-dont-like-my-code-what-can-i-do/117090/1 "2007-10-12T20:41:21Z")
(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! ---
**Author:** ![dewie](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/d/ac8455/32.png) [@dewie](https://www.ruby-forum.com/u/dewie)\ **Post date:** [October 12, 2007, 9:08pm UTC](https://www.ruby-forum.com/t/i-dont-like-my-code-what-can-i-do/117090/2 "2007-10-12T21:08:02Z")
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. ---
**Author:** ![dewie](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/d/ac8455/32.png) [@dewie](https://www.ruby-forum.com/u/dewie)\ **Post date:** [October 12, 2007, 9:41pm UTC](https://www.ruby-forum.com/t/i-dont-like-my-code-what-can-i-do/117090/3 "2007-10-12T21:41:48Z")
John J. wrote: > 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! ---
**Author:** ![dewie](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/d/ac8455/32.png) [@dewie](https://www.ruby-forum.com/u/dewie)\ **Post date:** [October 12, 2007, 10:02pm UTC](https://www.ruby-forum.com/t/i-dont-like-my-code-what-can-i-do/117090/4 "2007-10-12T22:02:26Z")
On Oct 12, 2007, at 2:41 PM, Daniel W. wrote: > > You’re trying to do too much with one model. (that’s more like C > > be confusing. is a bid the price offered by a vendor? or is a bid > > 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! > > Posted via [http://www.ruby-forum.com/](http://www.ruby-forum.com/). 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. ---
**Author:** ![dewie](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/d/ac8455/32.png) [@dewie](https://www.ruby-forum.com/u/dewie)\ **Post date:** [October 13, 2007, 1:15am UTC](https://www.ruby-forum.com/t/i-dont-like-my-code-what-can-i-do/117090/5 "2007-10-13T01:15:32Z")
John J. wrote: > 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.) ---
**Author:** ![dewie](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/d/ac8455/32.png) [@dewie](https://www.ruby-forum.com/u/dewie)\ **Post date:** [October 13, 2007, 4:02am UTC](https://www.ruby-forum.com/t/i-dont-like-my-code-what-can-i-do/117090/6 "2007-10-13T04:02:41Z")
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. ---
**Author:** ![dewie](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/d/ac8455/32.png) [@dewie](https://www.ruby-forum.com/u/dewie)\ **Post date:** [October 13, 2007, 3:18am UTC](https://www.ruby-forum.com/t/i-dont-like-my-code-what-can-i-do/117090/7 "2007-10-13T03:18:16Z")
I think that other guy should be fired and I should be hired 🙂 ---
**Author:** ![dewie](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/d/ac8455/32.png) [@dewie](https://www.ruby-forum.com/u/dewie)\ **Post date:** [October 13, 2007, 12:38pm UTC](https://www.ruby-forum.com/t/i-dont-like-my-code-what-can-i-do/117090/8 "2007-10-13T12:38:10Z")
Daniel W. wrote: > 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