tag:blogger.com,1999:blog-8795604888160975763.post127795707157611199..comments2023-09-25T12:19:02.322+02:00Comments on Solid Craft: Boy Scout Rule in practicejnbhttp://www.blogger.com/profile/11945481488520599011noreply@blogger.comBlogger9125tag:blogger.com,1999:blog-8795604888160975763.post-24193294298993110182013-01-03T23:18:52.587+01:002013-01-03T23:18:52.587+01:00This comment has been removed by the author.Samuelhttps://www.blogger.com/profile/18140931118197628245noreply@blogger.comtag:blogger.com,1999:blog-8795604888160975763.post-21211157170529021952010-05-31T19:27:38.682+02:002010-05-31T19:27:38.682+02:00This comment has been removed by the author.Yarcohttps://www.blogger.com/profile/15001871561109038466noreply@blogger.comtag:blogger.com,1999:blog-8795604888160975763.post-63154149182167128322010-05-30T23:17:37.415+02:002010-05-30T23:17:37.415+02:00The "inheritance is bad" mantra has aris...The "inheritance is bad" mantra has arisen because of early experiences with object orientated systems where complicated inheritance graphs were built up just as a means of additional layers of behaviour. This results in very fragile code, specially when methods invoke other methods in the same class which may be overridden further down the chain.<br /><br />REPEATEDLY extending objects just to alter their behavior is dangerous. Doing it once to fairly anaemic domain objects or just DTO's is not so dangerous, specially if you are just adding a constructor which configures the object differently, or adding independent utility methods. You have to be pragmatic about the cleanest way to solve a problem.<br /><br />Yes, as you say doing the configuration in the constructor is also fine, and generally I would do that. You could also just use a factory for creation of different types of orders. The basic idea is the same though - the actual code for constructing an Order from a SalesOrderExportDetail should not be in the middle of a loop over a list of SalesOrderExportDetails which has just been pulled from a webservice (imo). In this example, I suggested overloading the methods because as per your initial task i'd have added the "public boolean inShippingZone() {}" to the ImportedOrder class to be used prior to being saved. In that case none of the vat calculations etc would need to be done until the Order was saved, hence the overridden methods.<br /><br />(I'm aware that could potentially cause strange behaviour and would thus keep the class private or package private and document it. If Order generation from a SalesOrderExportDetail was also likely to done elsewhere, that would probably mean there should be either a factory for Orders with a corresponding method or a new constructor taking a SalesOrderExportDetail)dbhttps://www.blogger.com/profile/01507987064721846650noreply@blogger.comtag:blogger.com,1999:blog-8795604888160975763.post-50155226092865017202010-05-29T13:29:34.849+02:002010-05-29T13:29:34.849+02:00@db: thanks for spotting the bug
As for the nuisa...@db: thanks for spotting the bug<br /><br />As for the nuisance/joke of 1-7 lines per method: that's what I thought about it when I've heard it the first time, about five years ago (not from Robert C. Martin but from my team leader). <br /><br />After reading "Clean Code" I've decided to finally give it a try, and half a year of doing this kind of "extract-till-you-drop" (term taken from http://www.infoq.com/presentations/Robert-C.-Martin-Bad-Code), I can say that it works extremely well in real life projects. The people I've been pairing with were also quite happy (though surprised at first). Try it and maybe it won't be a nuisance anymore.<br /><br />The idea of ImportedOrder is interesting, but overriding getters means you have to do the calculation every time they are called or... you could store results, right? <br /><br />Correct me if I'm wrong, but if you want to store results, the best place is probably in the original Order object (so that the ImportedOrder class has got only constructor inside, that fires calculations). <br /><br />But then... it doesn't have to inherit from Order at all, does it? It could be a class with a single method returning the Order. We all know that inheritance is bad after all.<br /><br />But then... it shouldn't be named ImportedOrder. It does the conversion and firing up of calculations, so we could call it something like OrderFactory or... OrderConverter.<br /><br />But that looks pretty much like first three lines of importOne(SalesOrderDto salesOrderDto) method, except the calculations are fired inside the converter. So maybe it's about giving the converter the responsibility of firing up the calculations? We have to choose between OrdersImporter and OrderConverter. I'd rather stay with OrdersImporter, because conversion imho does not imply vat calculations, while importing an order may.<br /><br />But then... I haven't changed anything.<br /><br />Ok, I've tried to implement your idea and came back to the starting point, but it is VERY possible I'm too much fixed on my solution (the author usually is). Could you show your example?jnbhttps://www.blogger.com/profile/11945481488520599011noreply@blogger.comtag:blogger.com,1999:blog-8795604888160975763.post-4951813704836559652010-05-29T12:18:08.974+02:002010-05-29T12:18:08.974+02:00This comment has been removed by the author.dbhttps://www.blogger.com/profile/01507987064721846650noreply@blogger.comtag:blogger.com,1999:blog-8795604888160975763.post-71167688512371351532010-05-29T12:14:38.169+02:002010-05-29T12:14:38.169+02:00There's a bug in the code.
order.setVatValue(v...There's a bug in the code.<br />order.setVatValue(vat);<br />should be<br />order.setVatValue(value);<br /><br />Personally I think that if you do that kind of refactoring when you are asked to do some other small task you're a "busybody" nuisance to your team. Pronouncements by Robert C. Martin on the correct size of methods being 1-7 lines are a joke, and should be ignored. Methods are too large when they become non scannable and/or start to become a source of errors. The original method was imo fine and should have been left alone.<br /><br />One far better improvement though which you have not mentioned would be to extend the Order object to create an ImportedOrder object, which takes a SalesOrderExportDetail as its constructor. Then the body of the loop just becomes:<br />orderRepository.save(new ImportedOrder(salesOrder));<br />with the<br />getValue<br />getAddress<br />getBuyerAddress<br />getVatRate<br />getVatValue<br />getPln<br />methods all overridden to return the correct value.dbhttps://www.blogger.com/profile/01507987064721846650noreply@blogger.comtag:blogger.com,1999:blog-8795604888160975763.post-28020743551624238632010-05-14T00:51:16.219+02:002010-05-14T00:51:16.219+02:00@Tomek
You are absolutely right.
In fact, as Ro...@Tomek<br /><br />You are absolutely right. <br /><br />In fact, as Robert C. Martin shows, whenever we have ANY comments in our code, it means we have failed in making the code self-explanatory. We should re-think our design and see if there is a way to express the meaning of the comment in the code alone.<br /><br />The only situation when we should have comments (javadocs), is when we create public APIs. Public library should be well documented because not everyone wants (or is able) to download and look into the source code.<br /><br />The example of code to be refactored (original interface and 30 lines method), is not the code I'd suggest anyone to write today. It's the code you, as a developer, may see in some project. <br /><br />I'm very happy that you have spotted this flaw in the interface. It's not the point of this post, but it's very important. Unfortunately I see this kind of "documentation" all around. You probably have also seen javadocs for setters and getters, telling you that these are setters and getter. This is how bad javadocs can get.<br /><br />@ Jacek<br /><br />“dateOfImportStartedAt” may be a better name, but frankly speaking, in my opinion it's a matter of taste. You could also call it “importBeginning” and the type (Date) would tell you what kind of beginning it is. After all, the type of a variable is as much important as the name.<br /><br />I'm still not sure what to choose in this kind of a situation. Some people are strongly against veryLongNamesThatHaveALotOfUnimportantStuffInside. Usually I'm happy as long as the information is there, and is readable.<br /><br />As for the repeating of “Order” in the name of the method and in the type/name of the variable, it's very interesting. My view is, that OrderConverter, like most converters, can have a lot of methods, and with converters, the name of the method usually says what is the output. For example the java.math.BigDecimal has an intValue() method. Same thing for toString().<br /><br />On the other hand, I believe that the type of the parameter is also very important, so for different types of methods I usually get rid of the type in method name.<br /><br />For example, instead of writing:<br /><br />Order.addLineItem( LineItem lineItem );<br /><br />I'd write:<br /><br />Order.add( LineItem lineItem) ;<br /><br />What do you think about it, Jacek?<br /><br />BTW: where are you Jacek, right now? I'm currently at Geecon in Poznań, and I thought I'd meet you there. Jakub Kurlenda said that he has seen your pass card at the entrance. Will you be here on the second day?jnbhttps://www.blogger.com/profile/11945481488520599011noreply@blogger.comtag:blogger.com,1999:blog-8795604888160975763.post-41523776673147155782010-05-13T21:19:22.275+02:002010-05-13T21:19:22.275+02:00Maybe off-topic, but still..
It took me some time...Maybe off-topic, but still..<br /><br />It took me some time to understand that there is no value in such javadocs like the ones you write. No offense, I used to write them like this myself for a long time because of the 11th commandment - 'a method must have a javadoc':<br />/**<br /> * Imports orders.<br /> */<br />public void importOrders();<br /><br />In fact, there is no value there, rather a violation of DRY principle.<br /><br />Yeah, it is nice to have javadocs but ONLY IF they say something more then the method name.<br /><br />Just my 3 cents.<br /><br />--<br />Tomek<br />http://kaczanowscy.pl/tomekTomekhttps://www.blogger.com/profile/00975651968503643202noreply@blogger.comtag:blogger.com,1999:blog-8795604888160975763.post-37763495383353068952010-05-13T21:09:12.159+02:002010-05-13T21:09:12.159+02:00Like it very much. It's a nice addition to my ...Like it very much. It's a nice addition to my reading of the book "Growing Object-Oriented Software, Guided by Tests". The blog entry reminds me some parts of the book very clearly. Thanks for sharing your take on it.<br /><br />Just a thought I've come up with during the lecture - what about replacing dateImportStartedAt into dateOfImportStartedAt (with Of)? Would it make the intent clearer?<br /><br />I can't live with orderConverter.toOrder(salesOrderDto) too. I thought about changing it to orderConverter.to(salesOrderDto), but it doesn't look well either. Somehow I don't like repeating Order in the variable name and its converter method. I wish I could change the name, but don't know how.Anonymoushttps://www.blogger.com/profile/09734540973692423017noreply@blogger.com