Timeline for Put conditional logic inside method for DRY, or keep it outside for readability?
Current License: CC BY-SA 3.0
19 events
| when toggle format | what | by | license | comment | |
|---|---|---|---|---|---|
| Mar 20, 2018 at 12:38 | answer | added | CharonX | timeline score: 0 | |
| Mar 20, 2018 at 5:41 | answer | added | candied_orange | timeline score: 0 | |
| Mar 19, 2018 at 22:18 | comment | added | Chris Cirefice | @RobertHarvey Actually you're right, I guess I didn't think of that approach. Simply wrapping the conditions themselves into their own method in the AdHandler class would be a really good compromise in this situation. I don't really like the method naming, and that's one thing I was trying to avoid (clunky-sounding names), but I think it's worth finding a happy medium in this case. You could make your comment an answer too ;) | |
| Mar 19, 2018 at 21:20 | comment | added | πάντα ῥεῖ | @RobertHarvey Well, not to mention that (depending on language), lambda expressions could get useful for generalization flexibility. | |
| Mar 19, 2018 at 20:49 | comment | added | Robert Harvey | Strictly speaking, DRY does not apply to mere method calls, but only to the method bodies. You would only consolidate method calls if you wanted to compose functionality, as in void showInterstitialAdConditionally() { if (!isSubscriptionActive() && isEventThresholdMet()) { showInterstitialAd(); } } Of course, you would need a new composition class that pulls together all of the necessary classes to make this happen. | |
| Mar 19, 2018 at 20:44 | history | edited | Chris Cirefice | CC BY-SA 3.0 | Made it obvious conditionals and showInterstitialAd() don't belong to the same class |
| Mar 19, 2018 at 20:42 | comment | added | Chris Cirefice | @RobertHarvey Ah, this is me shortening the code too much and making a mess of the whole question! isSubscriptionActive() and isEventThresholdMet() are actually not part of the same class. They are in two different classes from where showInterstitialAd() is located. My bad, I'm really tired. I'll edit the question to make that apparent... | |
| Mar 19, 2018 at 20:39 | comment | added | Robert Harvey | I know. But this is a "don't worry about it" situation. Put your conditional methods in the same class as showInterstitialAd(), and you're good to go. | |
| Mar 19, 2018 at 20:38 | history | edited | Chris Cirefice | edited tags | |
| Mar 19, 2018 at 20:38 | comment | added | Chris Cirefice | @RobertHarvey Not true, sadly. This interstitial ad method is called in at least 6 different classes (Android activities/screens) at the moment, and may grow in the future. So if I go with version 2, I'll have a conditional for each class calling showInterstitialAd(). I replaced the [conditionals] tag with [android], though I'm not sure if Android actually matters here. But it's more descriptive anyway. | |
| Mar 19, 2018 at 20:35 | comment | added | Robert Harvey | Then the methods in question are likely to appear within the same class anyway. I don't buy that this is going to be hard to follow because other developers aren't going to look at the relevant code. It's all right there, in one class. | |
| Mar 19, 2018 at 20:35 | history | edited | Chris Cirefice | edited tags | |
| Mar 19, 2018 at 20:35 | answer | added | Bent | timeline score: -3 | |
| Mar 19, 2018 at 20:35 | comment | added | Chris Cirefice | @RobertHarvey whoops, totally failed on that one lol. It's Java. | |
| Mar 19, 2018 at 20:31 | answer | added | Deacon | timeline score: 8 | |
| Mar 19, 2018 at 20:30 | comment | added | Robert Harvey | Yes, it is. Different languages have different idioms. | |
| Mar 19, 2018 at 20:29 | comment | added | πάντα ῥεῖ | @RobertHarvey Looks like java or c#. Is that really relevant for the overall design? | |
| Mar 19, 2018 at 20:28 | comment | added | Robert Harvey | Which programming language? | |
| Mar 19, 2018 at 20:27 | history | asked | Chris Cirefice | CC BY-SA 3.0 |