• Post Reply Bookmark Topic Watch Topic
  • New Topic

Design - bad inheritance  RSS feed

 
Prasanna Raman
Ranch Hand
Posts: 410
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I have been put on a project aiming to create a brand new 'prequalification' application for a credit card, where we take only a subset of the information we usually take when someone applies for a credit card. The output is a list of 'offers' which they can then apply for (via a normal application).

There are currently already other types of applications in existence.

When I look at existing code, I see very bad inheritance. For example,  the inheritance has been done based on the flow of the web pages.

There is a 'personal' information class which is extended by something like 'other information'.

In another flow, 'personal information' is extended by 'some other information'.

And 'personal information' itself extends some basic 'application' which contains a lot of fields not needed by its subclasses.

My 'prequalification' class shares a lot of fields with all the existing classes but every one of them contains too much data than what I need.

How should I approach this? I certainly want to do this the right way without having to rewrite any of the existing code. Kindly suggest.

If this question is vague, I can give more information.
 
Prasanna Raman
Ranch Hand
Posts: 410
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
One more thing to add is that this whole system is just a pass through; that is, it doesn't act on the data received, it only does basic validation and passes it on to a back end service.

So, all these classes that I mentioned don't contain any useful methods apart from getters and setters.
 
Junilu Lacar
Sheriff
Posts: 11490
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Prasanna Raman wrote:
How should I approach this? I certainly want to do this the right way without having to rewrite any of the existing code. Kindly suggest.

I don't know what kind of advice you're expecting. Aside from not having enough context or insight into exactly what "this" is that you're trying to find a good approach to, you're giving the constraint of doing "it" the right way and yet not rewriting any of the existing code. That's kind of like asking how you can eat your cake and still have it. From what you describe, I would think it realistic to expect at least some refactoring if you want to avoid putting crap code/design over an already crappy codebase/design.

What exactly is "this" that you're trying to do? I understand the part about creating a brand new pre-qualification application but that's kind of like saying you need a new outfit without telling us what activity you need this outfit for. You're going to have to give more context.

What consumes these existing application type objects now? Do you have to pass your new application type to the same consumer? Can you create a different consumer? If you can create a separate path for incoming application data to follow, one that's designed better for at least part of the flow, then that might be a course of action you could explore. This is assuming that there are clear and clean seams in your current design where you can branch off to an alternate flow and then merge back downstream to the existing path. Bad designs often don't give you the luxury to do that though. In my experience, bad designs are bad across the board and they thwart your every attempt to do the right thing, no matter how deep you dig or how hard you try to find a way. But then again, I just don't think there's enough information to give you concrete and actionable advice.
 
Prasanna Raman
Ranch Hand
Posts: 410
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you very much for your response, Junilu.
Junilu Lacar wrote:
I don't know what kind of advice you're expecting. Aside from not having enough context or insight into exactly what "this" is that you're trying to find a good approach to
Sorry about the vague 1st post, I'll try to be more clear this time.


Junilu Lacar wrote:

That's kind of like asking how you can eat your cake and still have it. From what you describe, I would think it realistic to expect at least some refactoring if you want to avoid putting crap code/design over an already crappy codebase/design.
Sure, I understand. The only reason I said that was because I am not sure how much 'rewriting' of the existing code I can do as part of this 'project' that I am in because of current deadlines, and also because that would create a need to re-test existing flows. I don't know if I'll be allowed to do that. Having said that, I am willing to do what's needed to not add more messy code into the system.


Junilu Lacar wrote:What exactly is "this" that you're trying to do?
My ask is to create a new object to 'hold' the data that the customer would input into this 'prequalification' form, and do a validation check on all the input fields.

Junilu Lacar wrote:
What consumes these existing application type objects now? Do you have to pass your new application type to the same consumer? Can you create a different consumer?
The consumer is a service which takes the data and passes it to another system for decisioning. I don't have control over the consumer, and I can't create a new one. And yes, it's the same consumer.


I'll try to give some more context by explaning the current model:



Then, there are 2 different methods depending on which flow the customer is in to pass data to service:



The class names are not real, I made them up for the sake of this post, but the real names are equally terrible nonetheless.

Unfortunately, this is the existing set up. Now, the ''prequalification" object that I need is only a subset of "CompleteForm" and "AnotherCompleteForm". There may be 1 or 2 new fields, but 90% of the fields exist in one of the current classes.

I am so confused as to how to start my design. Inherting any of the existing classes seems a bad idea. On the other hand, creating a whole new class with the same fields would also be duplicating existing code.

Hope this post is more clear. I can give more information if necessary.
 
Junilu Lacar
Sheriff
Posts: 11490
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Well, unfortunately, you seem to be facing a situation many well-meaning developers find themselves facing when deadlines and a short-term outlook are given more importance than quality and a long-term outlook. As I said earlier, bad designs will fight and thwart any developer who tries to do the right thing, especially when the schedule and deadlines are not on that developer's side.  If this new form you're supposed to create is just a shell object that shuttles data around and that's what you need to use, I suppose you're going to have to bite the bullet and do it even though you know it's not the best thing to do for the long-term health of the code.

If it would make you feel better, you could go to a lead developer or architect and raise your concerns about the design. If you're lucky, they might listen and lend you a sympathetic ear. Best case, they'll try to work with you and find some way to start making things better. Worst case, they'd reject your concerns and say this is a good design. If it was me and the worst case happens, I'd probably start looking for another job. Not saying that you should do that because I don't know what your situation is but a project where quality plays second or third fiddle is bound to start grinding down to a crawl or suddenly crash and burn necessitating an "all hands on deck" fire-fighting exercise and any number of sleepless nights and days of blame-putting and finger-pointing to follow. Sorry to paint such a bleak picture but I've seen this sort of thing too many times and have been in a few of them myself. It's not fun and I'd never wish it on anybody.

Good luck.
 
Prasanna Raman
Ranch Hand
Posts: 410
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you, Junilu. I have tried to bring this up earlier, and people have acknowledged it's not ideal but unfortunately as you said, there's far too much focus on getting more and more things done and quality takes a back seat. No one cares.

Now, if you'd be kindly put up with me and help me through this, I want to take this as an opportunity to learn some good design principles. Just for learning's sake, I want to try and shot at refactoring this code and get it to a state where future changes are not such a pain. Do you think that would be a good exercise, or is this code beyond repair already?

If it can still be redone, kindly give me some pointers.
 
Junilu Lacar
Sheriff
Posts: 11490
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'm always up for an opportunity to talk about design principles, refactoring, and unit testing. I assume you want to write some unit tests as well...

Show us some code and we'll get into more details.
 
Prasanna Raman
Ranch Hand
Posts: 410
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Here's my initial design:





Does this look OK?
 
Junilu Lacar
Sheriff
Posts: 11490
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Prasanna Raman wrote:Here's my initial design:
...
Does this look OK?

I'm going to refer you to what I replied here because the same idea applies: https://coderanch.com/forums/posts/preList/676192/3168609#3168609

You might also want to go through most of that thread and pick out some more ideas about how to go about figuring out an appropriate workable design.
 
Prasanna Raman
Ranch Hand
Posts: 410
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you, Junilu. That thread has a lot of good information. I will keep referring back to it.

One of my problems is that I my application doesn't need to perform any functions, and so I am not able to think in terms of behaviour first and then derive my attributes from there. All I am doing is passing all the information the user is entering on a form to a back end service.

Here's how my classes would interact



Please review.
 
Junilu Lacar
Sheriff
Posts: 11490
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Prasanna Raman wrote:
Here's how my classes would interact
...
Please review.

There's not really much interaction there, so not much to review either. That's the problem with anemic object models that only have data and connections to other objects but very little behavior.
 
Prasanna Raman
Ranch Hand
Posts: 410
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you, Junilu for the link to a search on anaemic data model. I found this really hard hitting from Martin Fowler: "In general, the more behavior you find in the services, the more likely you are to be robbing yourself of the benefits of a domain model. If all your logic is in services, you've robbed yourself blind."

Unfortunately, I am not in a position to change all that with my current project. However, I have already brought this up a couple of times with the architects and I think they seem to acknowledge there's a problem.

Within my limitations, I would still to design this as best as possible (if you can really call this design at all).

Note that I used Composition in my classes; I have a PersonalInformation object and a StudentInformation object within my Prequalification object.

How does this approach compare with using inheritance instead? Create a StudentInformation object, then a subclass called PrequalificationStudentInformation, which will be used within Prequlification object. So that if in the future, there's more shared classes and if they had more shared fields, we could add them to StudentInformation class, and inherit all the common fields. The same concept can be applied to the PeronalInformation object.
 
Junilu Lacar
Sheriff
Posts: 11490
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You should take comfort in knowing that Joshua Bloch recommends that you favor composition over inheritance in his book, Effective Java.
 
Junilu Lacar
Sheriff
Posts: 11490
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
If you post some unit test code, I could give you more feedback.
 
Prasanna Raman
Ranch Hand
Posts: 410
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you, I will post some tests soon.

I just ran into one of the architects here. He looked at my code and said that having composite objects (like PersonalInformation, StudentInformation) doesn't make sense here because it's not worth it. Instead, he said I should just keep all the fields individually because that's how it is in the other existing classes.

I was arguing that we might be able to share these fields between classes, if in the future we had to create other classes needing the same fields,  but he felt it was no big deal to duplicate those fields there because the application didn't have any behaviour; it was just a pass-through - receive data from the front end and pass it on to a service.

His argument didn't sound convincing to me; I see no reason why there shouldn't be composite objects.

Could you please comment on this?
 
Junilu Lacar
Sheriff
Posts: 11490
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Prasanna Raman wrote:I just ran into one of the architects here. He looked at my code and said that having composite objects (like PersonalInformation, StudentInformation) doesn't make sense here because it's not worth it. Instead, he said I should just keep all the fields individually because that's how it is in the other existing classes.

(ლ.–)

All I can think of is "the banality of poor design..."

Could you please comment on this?

If we can't say anything nice or constructive, we're not supposed to comment at all.  I think I already said enough to almost get in trouble so I'll leave it at that.  ¯\_(ツ)_/¯
 
Prasanna Raman
Ranch Hand
Posts: 410
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Sorry to keep harping on the subject, but just for my own understanding and peace of mind, I will just ask this:

Is there any merit to the argument that there shouldn't be any composite objects because the application is just a pass through and doesn't contain any behaviour? I know this whole model itself is flawed, but I was just trying to do my best given the limitations.

His argument was why add an extra layer of complexity when the whole application was just a place holder, even if it means a new class tomorrow would have to declare all these fields separately again despite there being a number of common fields.

I ask this just for my own understanding, because I am so disappointed right now with this.
 
Junilu Lacar
Sheriff
Posts: 11490
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Prasanna Raman wrote:Is there any merit to the argument that there shouldn't be any composite objects because the application is just a pass through and doesn't contain any behaviour? I know this whole model itself is flawed, but I was just trying to do my best given the limitations.

You're probably asking the wrong person about this. I have a very strong bias for quality.  I *CAN NOT* abide with design decisions that are made for expediency and consistency when there are obvious violations of principles of good software design.

Just to give you an idea of how serious I am about good designs: When I am doing pair or group programming with other developers on my team, I will not allow the discussion to move forward if there is even one name that does not clearly and accurately express the intent of the code. We can spend up to 10 minutes discussing what a good name would be for a class, interface, enum, variable or method. That may sound extreme and a waste of time but we're not just talking about a name. We're talking about intent, overall design coherence and cohesiveness, about the objects and their interactions, responsibilities, etc.  I make my teammates look at the code from a high-level abstract perspective and make them relate, in their own words and in plain English, what they believe is happening in the code. I tell them pretty much the same things I tell everyone here on the ranch: Choose good names, reveal intent, make things small, separate concerns, and make your code, all the code, tell a coherent story.

I would not last long working with architects like the ones you're working with, and I've had to do that a number of times in the past in my career. None of those times was ever a party, I can tell you that. I'm just lucky now that I've found a place of employment where I pretty much have the last word when it comes to design decisions and the people I work with are all people I have interviewed thoroughly before I hire them. So while I make sure we have a diverse set of skills on our team, I also make sure we have one thing in common: a commitment to craftsmanship and doing things right, not just doing things fast. And because they all have an agile mindset, we all also believe in the agile adage, "The only way to go fast is to go well."

As far as your question as to there being merit to the argument the architect gave, well, if the main consideration for not having composite objects is because it's expedient and stays consistent with the current design then all I can say is "So if everybody walked around wearing their underwear on their heads, you'd do that, too?"
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!