tag:blogger.com,1999:blog-32241238.post9009627875313537489..comments2023-03-25T05:46:26.256-04:00Comments on Why not?: Why You Can Throw Away "If", and Why You Shouldn'tDanhttp://www.blogger.com/profile/06099373265709774874noreply@blogger.comBlogger9125tag:blogger.com,1999:blog-32241238.post-43148589591283736042008-09-04T00:58:00.000-04:002008-09-04T00:58:00.000-04:00Every create new and update view I've ever seen ha...Every create new and update view I've ever seen has had a few differences, even if it was as trivial as a title/header change. Reuse is a good thing of course if the forms really are displaying the exact same data, and I don't mean to discourage that at all, except when I see a developer using a lot of conditionals to handle the differences (I'd prefer two duplicated pages to one unreadable page cluttered with if statements -- really it's just a trade off between two different forms of duplication, but the duplicated ifs are harder to read, modify, and change independently).<BR/><BR/>My preference would be to reduce the page to a single if (or zero ifs using something like a rails partial), as you did, but to have that if surround the form url so that create went to a create link and update went to an update link that takes an id. That way, the decision can be made once, on page rendering time, that we are doing an update, not twice (when detecting the presence or absence of an id), or multiple times (as it was before your refactoring). This may avoid problems as well; for example, what happens if a bug causes the id to not appear, or if the id's field's name changed in the view but not the request handler? Would the code mistake an update for a create?<BR/><BR/>But it sounds like you already vastly improved the code, so my suggestions might not be a big win now compared to other needs that the codebase may have.Anonymousnoreply@blogger.comtag:blogger.com,1999:blog-32241238.post-76139780008986677132008-09-02T19:26:00.000-04:002008-09-02T19:26:00.000-04:00In my case, I had one JSP that was using an inline...In my case, I had one JSP that was using an inline if statment to choose which URL it should use to post. In addition, it used further conditionals to figure out what form fields to render into the form (sometimes it would include various hidden form fields, sometimes it would not). In the end, I ended up getting rid of all the conditions except one around the hidden form field itemId. That way, the various pieces of data are always posted to the action, and the action uses the presence of the itemId field to differentiate between a create and an update.<BR/><BR/>I don't yet quite get REST, but I think that the RESTful approach would be to make the update URL different from the create URL. In particular, the item ID should be part of the update URL, whereas it can't be part of the create URL.<BR/><BR/>Maybe it's just me, but I see a lot of similarity between creating and updating. Both take mostly the same data, and do mostly the same things with that data. In most interfaces that I've seen, the user is presented with the same fields. That's not universally true, but it's pretty close. This is why a lot of Rails apps use a shared partial.<BR/><BR/>But you're right, it probably makes sense to keep them separate, and re-use code from the inside-out (i.e. by calling shared methods, rather than using a shared wrapper). No matter what, there is a lot less duplication now than there was before. That's the thing that really matters.Danhttps://www.blogger.com/profile/06099373265709774874noreply@blogger.comtag:blogger.com,1999:blog-32241238.post-50908283414791350162008-09-01T14:55:00.000-04:002008-09-01T14:55:00.000-04:00Do you have something like an update.jsp and a cre...Do you have something like an update.jsp and a createnew.jsp, or do both operations share one jsp?<BR/><BR/>I find that trying to keep your update and create new views/controllers the same tends to cause all sorts of problems since there will inevitably be display and logic differences between creating and updating, and there is little reason they can't just be separated out. This is a huge advantage actually, since they have domain knowledge that the rest of the system would have to try to figure out, and this information can easily be passed down into the deeper layers of the system.<BR/><BR/>Your storeItem function may then look like:<BR/><BR/>function storeItem(persister) {<BR/> persister.update(function(item) {<BR/> item.name = request["name"];<BR/> item.description = request["description"];<BR/> item.quantity = request["quantity"];<BR/> item.value = someComplexCalculation();<BR/> item.totalValue = item.quantity * item.value;<BR/> // calculate and store some more fields here<BR/> });<BR/>}<BR/><BR/>Your various controllers just pass in an appropriate persister, a la:<BR/><BR/>createnew -<BR/>...<BR/>storeItem(new NewItemPersister());<BR/>...<BR/><BR/>update - <BR/>...<BR/>storeItem(new ExistingItemPersister());<BR/>...<BR/><BR/>This is more modular. For example, if particular items need to be persisted in several locations, you can aggregate the persister thusly:<BR/><BR/>storeItem(new MultiPersister(new DatabasePersister(), new FileSystemPersister(), new SecretOffsitePersonalInfomationPersister()));Anonymousnoreply@blogger.comtag:blogger.com,1999:blog-32241238.post-26264418772380961082008-08-16T16:24:00.000-04:002008-08-16T16:24:00.000-04:00I had been trying to unify both methods. I didn...I had been trying to unify both methods. I didn't want to force the JSP to decide which URL (and, thus, which method) to use. Instead, I wanted to push that decision down as far as possible. Given that, which of the following is better?<BR/><BR/>public void storeItem() {<BR/> if (request["itemId"] == null) {<BR/> Item item = new Item();<BR/> updateItemFields(item);<BR/> items.addNewItem(item);<BR/> } else {<BR/> Item item = items.get(request["itemId"]);<BR/> updateItemFields(item);<BR/> item.update();<BR/> }<BR/>}<BR/><BR/>void updateItemFields(Item item) {<BR/> item.name = request["name"];<BR/> item.description = request["description"];<BR/> item.quantity = request["quantity"];<BR/> item.value = someComplexCalculation();<BR/> item.totalValue = item.quantity * item.value;<BR/> // calculate and store some more fields here<BR/>}<BR/><BR/>or this<BR/><BR/>function storeItem() {<BR/> //this first section could actually be replaced with custom parameter binding rules<BR/> if (request["itemId"] == null) {<BR/> var persister = newItemPersister;<BR/> } else {<BR/> var persister = new ExistingItemPersister(request["itemId"]);<BR/> }<BR/> <BR/> persister.update(function(item) {<BR/> item.name = request["name"];<BR/> item.description = request["description"];<BR/> item.quantity = request["quantity"];<BR/> item.value = someComplexCalculation();<BR/> item.totalValue = item.quantity * item.value;<BR/> // calculate and store some more fields here<BR/> });<BR/>}<BR/><BR/>I find the second to be easier to understand. Everybody has their preference, but neither of these is absolutely better than the other.Danhttps://www.blogger.com/profile/06099373265709774874noreply@blogger.comtag:blogger.com,1999:blog-32241238.post-13718778443945269182008-08-15T09:57:00.000-04:002008-08-15T09:57:00.000-04:00public void storeNewItem() { Item item = new It...public void storeNewItem() {<BR/> Item item = new Item();<BR/> updateItemFields(item);<BR/> items.addNewItem(item);<BR/>}<BR/><BR/>public void storeExistingItem() {<BR/> Item item = items.get(request["itemId"]);<BR/> updateItemFields(item);<BR/> item.update();<BR/>}<BR/><BR/>void updateItemFields(Item item)<BR/>{<BR/> item.name = request["name"];<BR/> item.description = request["description"];<BR/> item.quantity = request["quantity"];<BR/> item.value = someComplexCalculation();<BR/> item.totalValue = item.quantity * item.value;<BR/> // calculate and store some more fields here<BR/>}Anonymousnoreply@blogger.comtag:blogger.com,1999:blog-32241238.post-85846372180361251702008-08-15T09:25:00.000-04:002008-08-15T09:25:00.000-04:00Hm well okay you're dealing with database crap. W...Hm well okay you're dealing with database crap. Well then your code will be at least a little bit ugly. QEDAnonymousnoreply@blogger.comtag:blogger.com,1999:blog-32241238.post-289846495860833742008-08-15T09:21:00.000-04:002008-08-15T09:21:00.000-04:00Haha yeah that was what I was thinking--except wit...Haha yeah that was what I was thinking--except without the super-slick ||. I would have wrapped the items cache in a class, not just the "persisting" operation. Maybe this would have changed the lifetime of an item or whatever, but you don't explain the significance of that issue in your post. Does the change in item lifetime hurt something? Or is it likely to start hurting something in a way that will be hard to track down when it happens? If not, don't worry about it.Anonymousnoreply@blogger.comtag:blogger.com,1999:blog-32241238.post-71584452729027253432008-08-15T09:20:00.000-04:002008-08-15T09:20:00.000-04:00Let me start by saying that your approach is certa...Let me start by saying that your approach is certainly simpler. I like that. The primary difference is that you are immediately storing a new item in the database. After the first line, there will be an item in the database for sure, so everything after that line can be written from the point of view of updating an item, rather than storing a new item.<BR/><BR/>I see two potential problems. The first is that the first line wouldn't compile in Java. You can use || with arbitrary objects in Javascript or Ruby, but Java would complain because it's not operating on booleans. (<A HREF="http://java.sun.com/docs/books/jls/second_edition/html/expressions.doc.html#54532" REL="nofollow">From the JLS</A>: "Each operand of || must be of type boolean, or a compile-time error occurs. The type of a conditional-or expression is always boolean."). This is not a big deal, since you can replace it with something that will work in Java. It's just not going to be quite so concise.<BR/><BR/>The other problem is that this code assumes that you can add empty items to the 'items' object. Something that may not have been clear in my original example is that items.addNewItem and item.update actually do eventually get into database persistence code. If you database has NOT NULL constraints, or if for some other reason an empty object is illegal in your schema, this code won't work at runtime.<BR/><BR/>In my case, I was taking some existing code and trying to change its structure while maintaining its behavior. I don't know what would happen if I tried to save an empty object, and that wasn't a can or worms that I wanted to open. <BR/><BR/>Finally, there's something that I like about making the code behave in a simple way, even if it may make the code itself more complex. Suppose this were the real world, and instead of populating an item, I were filling out a tax form. In the real world, it would not make sense for me to submit an empty form, then ask for the form back so that I can fill it out. I would fill it out before submitting it the first time. This is purely an aesthetic choice, and in this case it's a pretty weak argument for making the code overly complicated. However, when code behaves in a simple, intuitive way, it will be easier for new developers (or, in fact, your future self) to work with the code base.Danhttps://www.blogger.com/profile/06099373265709774874noreply@blogger.comtag:blogger.com,1999:blog-32241238.post-10492307138209005622008-08-15T04:27:00.000-04:002008-08-15T04:27:00.000-04:00Uhm.. Why not just:public void storeItem(){ item ...Uhm.. Why not just:<BR/><BR/>public void storeItem()<BR/>{<BR/> item = items.get(request["_itemid"]) || items.addNewItem(new Item);<BR/><BR/> item.name = request["name"];<BR/> item.description = request["description"];<BR/> item.quantity = request["quantity"];<BR/> item.value = someComplexCalculation();<BR/> item.totalValue = item.quantity * item.value;<BR/> // calculate and store some more fields here<BR/><BR/> item.update();<BR/>}Anonymousnoreply@blogger.com