How to solve code duplication in this example where I introduce inheritance to actually solve code...
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty{ height:90px;width:728px;box-sizing:border-box;
}
Suppose I have an App with different Scenes
. Each Scene has a layout that is build up by a number of ScreenElement
s, e.g., rectangular boxes that will be drawn to the Canvas
. For some Scenes
, the same boxes will be used. Therefore, in view of DRY, I want to use inheritance.
What I want to do is to store the ScreenElement
s in a HashMap
(or ultimately an EnumMap
) so that I can call on the ScreenElement
s elsewhere in my code to change their attributes.
Here is what I came up with now...
Here is the basic layout:
Public class BasicLayout {
private HashMap<String, ScreenElement> screenElements;
public BasicLayout() {
screenElements = new HashMap<>();
screenElements.put("BACKGROUND", new ScreenElement(...));
screenElements.put("BOXONE", new ScreenElement(...));
screenElements.put("BOXTWO", new ScreenElement(...));
}
public HashMap<String, ScreenElement> getScreenElements() {
return screenElements;
}
public ScreenElement getScreenElement(String elementName) {
return screenElements.get(elementName);
}
public void draw(Canvas canvas) {
for (ScreenElement screenElement: screenElements) {
screenElement.draw(canvas);
}
}
}
Then another layout could be something like:
Public class OtherLayout extends BasicLayout {
private HashMap<String, ScreenElement> screenElements;
public OtherLayout() {
screenElements = new HashMap<>(super.getScreenElements);
screenElements.put("BOXTHREE", new ScreenElement(...));
screenElements.put("BOXFOUR", new ScreenElement(...));
}
@Override
public HashMap<String, ScreenElement> getScreenElements() {
return screenElements;
}
@Override
public ScreenElement getScreenElement(String elementName) {
return screenElements.get(elementName);
}
@Override
public void draw(Canvas canvas) {
for (ScreenElement screenElement: screenElements) {
screenElement.draw(canvas);
}
}
So the thing is that by solving code duplication (I no longer need to add BACKGROUND, BOXONE and BOXTWO in my OtherLayout
), I introduce code duplication!
I now need to duplicate the getScreenElements
, getScreenElement
and draw
methods. In this case you need to override them, because if you don't, getScreenElements
, for example, always return BACKGROUND, BOXONE and BOXTWO, even when you actually want BACKGROUND, BOXONE, BOXTWO, BOXTHREE and BOXFOUR (for example in the Scene
that uses `OtherLayout'.
Hope this makes sense and that someone has an ingenious solution!
To clarify:
- At any time
BasicLayout
should have BACKGROUND, BOXONE and BOXTWO - At any time
OtherLayout
should have BACKGROUND, BOXONE, BOXTWO,
BOXTHREE and BOXFOUR.
java android inheritance dry
add a comment |
Suppose I have an App with different Scenes
. Each Scene has a layout that is build up by a number of ScreenElement
s, e.g., rectangular boxes that will be drawn to the Canvas
. For some Scenes
, the same boxes will be used. Therefore, in view of DRY, I want to use inheritance.
What I want to do is to store the ScreenElement
s in a HashMap
(or ultimately an EnumMap
) so that I can call on the ScreenElement
s elsewhere in my code to change their attributes.
Here is what I came up with now...
Here is the basic layout:
Public class BasicLayout {
private HashMap<String, ScreenElement> screenElements;
public BasicLayout() {
screenElements = new HashMap<>();
screenElements.put("BACKGROUND", new ScreenElement(...));
screenElements.put("BOXONE", new ScreenElement(...));
screenElements.put("BOXTWO", new ScreenElement(...));
}
public HashMap<String, ScreenElement> getScreenElements() {
return screenElements;
}
public ScreenElement getScreenElement(String elementName) {
return screenElements.get(elementName);
}
public void draw(Canvas canvas) {
for (ScreenElement screenElement: screenElements) {
screenElement.draw(canvas);
}
}
}
Then another layout could be something like:
Public class OtherLayout extends BasicLayout {
private HashMap<String, ScreenElement> screenElements;
public OtherLayout() {
screenElements = new HashMap<>(super.getScreenElements);
screenElements.put("BOXTHREE", new ScreenElement(...));
screenElements.put("BOXFOUR", new ScreenElement(...));
}
@Override
public HashMap<String, ScreenElement> getScreenElements() {
return screenElements;
}
@Override
public ScreenElement getScreenElement(String elementName) {
return screenElements.get(elementName);
}
@Override
public void draw(Canvas canvas) {
for (ScreenElement screenElement: screenElements) {
screenElement.draw(canvas);
}
}
So the thing is that by solving code duplication (I no longer need to add BACKGROUND, BOXONE and BOXTWO in my OtherLayout
), I introduce code duplication!
I now need to duplicate the getScreenElements
, getScreenElement
and draw
methods. In this case you need to override them, because if you don't, getScreenElements
, for example, always return BACKGROUND, BOXONE and BOXTWO, even when you actually want BACKGROUND, BOXONE, BOXTWO, BOXTHREE and BOXFOUR (for example in the Scene
that uses `OtherLayout'.
Hope this makes sense and that someone has an ingenious solution!
To clarify:
- At any time
BasicLayout
should have BACKGROUND, BOXONE and BOXTWO - At any time
OtherLayout
should have BACKGROUND, BOXONE, BOXTWO,
BOXTHREE and BOXFOUR.
java android inheritance dry
add a comment |
Suppose I have an App with different Scenes
. Each Scene has a layout that is build up by a number of ScreenElement
s, e.g., rectangular boxes that will be drawn to the Canvas
. For some Scenes
, the same boxes will be used. Therefore, in view of DRY, I want to use inheritance.
What I want to do is to store the ScreenElement
s in a HashMap
(or ultimately an EnumMap
) so that I can call on the ScreenElement
s elsewhere in my code to change their attributes.
Here is what I came up with now...
Here is the basic layout:
Public class BasicLayout {
private HashMap<String, ScreenElement> screenElements;
public BasicLayout() {
screenElements = new HashMap<>();
screenElements.put("BACKGROUND", new ScreenElement(...));
screenElements.put("BOXONE", new ScreenElement(...));
screenElements.put("BOXTWO", new ScreenElement(...));
}
public HashMap<String, ScreenElement> getScreenElements() {
return screenElements;
}
public ScreenElement getScreenElement(String elementName) {
return screenElements.get(elementName);
}
public void draw(Canvas canvas) {
for (ScreenElement screenElement: screenElements) {
screenElement.draw(canvas);
}
}
}
Then another layout could be something like:
Public class OtherLayout extends BasicLayout {
private HashMap<String, ScreenElement> screenElements;
public OtherLayout() {
screenElements = new HashMap<>(super.getScreenElements);
screenElements.put("BOXTHREE", new ScreenElement(...));
screenElements.put("BOXFOUR", new ScreenElement(...));
}
@Override
public HashMap<String, ScreenElement> getScreenElements() {
return screenElements;
}
@Override
public ScreenElement getScreenElement(String elementName) {
return screenElements.get(elementName);
}
@Override
public void draw(Canvas canvas) {
for (ScreenElement screenElement: screenElements) {
screenElement.draw(canvas);
}
}
So the thing is that by solving code duplication (I no longer need to add BACKGROUND, BOXONE and BOXTWO in my OtherLayout
), I introduce code duplication!
I now need to duplicate the getScreenElements
, getScreenElement
and draw
methods. In this case you need to override them, because if you don't, getScreenElements
, for example, always return BACKGROUND, BOXONE and BOXTWO, even when you actually want BACKGROUND, BOXONE, BOXTWO, BOXTHREE and BOXFOUR (for example in the Scene
that uses `OtherLayout'.
Hope this makes sense and that someone has an ingenious solution!
To clarify:
- At any time
BasicLayout
should have BACKGROUND, BOXONE and BOXTWO - At any time
OtherLayout
should have BACKGROUND, BOXONE, BOXTWO,
BOXTHREE and BOXFOUR.
java android inheritance dry
Suppose I have an App with different Scenes
. Each Scene has a layout that is build up by a number of ScreenElement
s, e.g., rectangular boxes that will be drawn to the Canvas
. For some Scenes
, the same boxes will be used. Therefore, in view of DRY, I want to use inheritance.
What I want to do is to store the ScreenElement
s in a HashMap
(or ultimately an EnumMap
) so that I can call on the ScreenElement
s elsewhere in my code to change their attributes.
Here is what I came up with now...
Here is the basic layout:
Public class BasicLayout {
private HashMap<String, ScreenElement> screenElements;
public BasicLayout() {
screenElements = new HashMap<>();
screenElements.put("BACKGROUND", new ScreenElement(...));
screenElements.put("BOXONE", new ScreenElement(...));
screenElements.put("BOXTWO", new ScreenElement(...));
}
public HashMap<String, ScreenElement> getScreenElements() {
return screenElements;
}
public ScreenElement getScreenElement(String elementName) {
return screenElements.get(elementName);
}
public void draw(Canvas canvas) {
for (ScreenElement screenElement: screenElements) {
screenElement.draw(canvas);
}
}
}
Then another layout could be something like:
Public class OtherLayout extends BasicLayout {
private HashMap<String, ScreenElement> screenElements;
public OtherLayout() {
screenElements = new HashMap<>(super.getScreenElements);
screenElements.put("BOXTHREE", new ScreenElement(...));
screenElements.put("BOXFOUR", new ScreenElement(...));
}
@Override
public HashMap<String, ScreenElement> getScreenElements() {
return screenElements;
}
@Override
public ScreenElement getScreenElement(String elementName) {
return screenElements.get(elementName);
}
@Override
public void draw(Canvas canvas) {
for (ScreenElement screenElement: screenElements) {
screenElement.draw(canvas);
}
}
So the thing is that by solving code duplication (I no longer need to add BACKGROUND, BOXONE and BOXTWO in my OtherLayout
), I introduce code duplication!
I now need to duplicate the getScreenElements
, getScreenElement
and draw
methods. In this case you need to override them, because if you don't, getScreenElements
, for example, always return BACKGROUND, BOXONE and BOXTWO, even when you actually want BACKGROUND, BOXONE, BOXTWO, BOXTHREE and BOXFOUR (for example in the Scene
that uses `OtherLayout'.
Hope this makes sense and that someone has an ingenious solution!
To clarify:
- At any time
BasicLayout
should have BACKGROUND, BOXONE and BOXTWO - At any time
OtherLayout
should have BACKGROUND, BOXONE, BOXTWO,
BOXTHREE and BOXFOUR.
java android inheritance dry
java android inheritance dry
edited Nov 16 '18 at 14:28
MWB
asked Nov 16 '18 at 13:07
MWBMWB
9831819
9831819
add a comment |
add a comment |
3 Answers
3
active
oldest
votes
This should work:
public class BasicLayout {
protected HashMap<String, ScreenElement> screenElements = new HashMap<>();
public BasicLayout()
{
screenElements.put("BACKGROUND", new ScreenElement(...));
screenElements.put("BOXONE", new ScreenElement(...));
screenElements.put("BOXTWO", new ScreenElement(...));
}
public HashMap<String, ScreenElement> getScreenElements() {
return screenElements;
}
public ScreenElement getScreenElement(String elementName) {
return screenElements.get(elementName);
}
public void draw(Canvas canvas) {
for (ScreenElement screenElement: screenElements) {
screenElement.draw(canvas);
}
}
}
public class OtherLayout extends BasicLayout {
public OtherLayout() {
screenElements.put("BOXTHREE", new ScreenElement(...));
screenElements.put("BOXFOUR", new ScreenElement(...));
}
}
If you create a BasicLayout u have BACKGROUND, BOXONE and BOXTWO, if you create a OtherLayout you have BACKGROUND, BOXONE, BOXTWO, BOXTHREE, BOXFOUR.
– Meini
Nov 16 '18 at 13:29
What you mean by go back? If you create an instance of a subclass doesn't mean u create two instances, so there is no shared screenElements at all.
– Meini
Nov 16 '18 at 13:39
I refactored my code and it works like a charm! Thank you very much!
– MWB
Nov 16 '18 at 13:54
add a comment |
I don't think solving inheritance is a good choice for avoiding duplication. You always violate Liskov substitution principle is some settle way (in your case not so settle:)). And that leads to far bigger problems down the line.
And the second point inheritance should be used only to share behaviour not data.
I think in your case the delegation/composition is a better fit:
public OtherLayout() {
screenElements = new HashMap<>();
screenElements.putAll(new BasicLayout().getScreenElements());
screenElements.put("BOXTHREE", new ScreenElement());
screenElements.put("BOXFOUR", new ScreenElement());
}
Because from the looks of it the only thing that is common for the two classes are the elements in the source elements map.
And a more general note. Don't be afraid of duplication as much as the wrong abstraction. The quote from the pioneer of modern computer science (Edsger Dijkstra):
The purpose of abstraction is not to be vague, but to create a new
semantic level in which one can be absolutely precise
You are right, I am primarily sharing data (and some behavior). In your solution, each different type of layout (I have not onlyOtherLayout
but alsoOtherLayout2
,OtherLayout3
,...) needs its owngetters
anddraw
methods, right? You don't see that as a problem? Especially if you have many layouts? Meini's answer works nicely and the code is nice and concise.
– MWB
Nov 16 '18 at 14:21
The solution might look good in terms of numbers of lines but to me the intention is not clear. I like to look at the code and know what's going on immediately. That is why I said that inheritance is not to share data. Suddenly you have to follow this fuzzy logic of state mutation that now spans 2 classes. And the tree of class hierarchy tends to get bigger, with time, not smaller.
– piotr szybicki
Nov 16 '18 at 15:18
The solution proposed by @Meini is very clever. But in my opinion it is too clever for it's own good :)
– piotr szybicki
Nov 16 '18 at 15:21
add a comment |
Based on the sample code, there only needs to be a single class which takes the elements it will be responsible for in the constructor. For example:
public class Layout {
private final Map<String, ScreenElement> screenElements;
public Layout(Map<String, ScreenElement> screenElements) {
this.screenElements = screenElements;
}
// other methods
}
From your example, somewhere in the code base would know to create a BasicLayout
or OtherLayout
. Refactor that code and extract the map, and pass to the constructor. For example:
// in some method that knows when to create a `BasicLayout` or `OtherLayout`
Map<String, ScreenElement> basicScreenElements = new HashMap<>();
basicScreenElements .put("BACKGROUND", new ScreenElement(...));
basicScreenElements .put("BOXONE", new ScreenElement(...));
basicScreenElements .put("BOXTWO", new ScreenElement(...));
Layout basicLayout = new Layout(basicScreenElements);
// ....
Map<String, ScreenElement> otherScreenElements = new HashMap<>();
otherScreenElements .put("BOXTHREE", new ScreenElement(...));
otherScreenElements .put("BOXFOUR", new ScreenElement(...));
Layout otherLayout = new Layout(otherScreenElements);
In your example,otherLayout
does not contain BACKGROUND, BOXONE and BOXTWO. If you want this, you need to add them, which leads to code duplication.
– MWB
Nov 16 '18 at 13:55
The creation of aScreenElement
object could be moved to a factory class, or provide aScreenElement
copy constructor.
– Andrew S
Nov 16 '18 at 14:04
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "1"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: true,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: 10,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fstackoverflow.com%2fquestions%2f53338543%2fhow-to-solve-code-duplication-in-this-example-where-i-introduce-inheritance-to-a%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
3 Answers
3
active
oldest
votes
3 Answers
3
active
oldest
votes
active
oldest
votes
active
oldest
votes
This should work:
public class BasicLayout {
protected HashMap<String, ScreenElement> screenElements = new HashMap<>();
public BasicLayout()
{
screenElements.put("BACKGROUND", new ScreenElement(...));
screenElements.put("BOXONE", new ScreenElement(...));
screenElements.put("BOXTWO", new ScreenElement(...));
}
public HashMap<String, ScreenElement> getScreenElements() {
return screenElements;
}
public ScreenElement getScreenElement(String elementName) {
return screenElements.get(elementName);
}
public void draw(Canvas canvas) {
for (ScreenElement screenElement: screenElements) {
screenElement.draw(canvas);
}
}
}
public class OtherLayout extends BasicLayout {
public OtherLayout() {
screenElements.put("BOXTHREE", new ScreenElement(...));
screenElements.put("BOXFOUR", new ScreenElement(...));
}
}
If you create a BasicLayout u have BACKGROUND, BOXONE and BOXTWO, if you create a OtherLayout you have BACKGROUND, BOXONE, BOXTWO, BOXTHREE, BOXFOUR.
– Meini
Nov 16 '18 at 13:29
What you mean by go back? If you create an instance of a subclass doesn't mean u create two instances, so there is no shared screenElements at all.
– Meini
Nov 16 '18 at 13:39
I refactored my code and it works like a charm! Thank you very much!
– MWB
Nov 16 '18 at 13:54
add a comment |
This should work:
public class BasicLayout {
protected HashMap<String, ScreenElement> screenElements = new HashMap<>();
public BasicLayout()
{
screenElements.put("BACKGROUND", new ScreenElement(...));
screenElements.put("BOXONE", new ScreenElement(...));
screenElements.put("BOXTWO", new ScreenElement(...));
}
public HashMap<String, ScreenElement> getScreenElements() {
return screenElements;
}
public ScreenElement getScreenElement(String elementName) {
return screenElements.get(elementName);
}
public void draw(Canvas canvas) {
for (ScreenElement screenElement: screenElements) {
screenElement.draw(canvas);
}
}
}
public class OtherLayout extends BasicLayout {
public OtherLayout() {
screenElements.put("BOXTHREE", new ScreenElement(...));
screenElements.put("BOXFOUR", new ScreenElement(...));
}
}
If you create a BasicLayout u have BACKGROUND, BOXONE and BOXTWO, if you create a OtherLayout you have BACKGROUND, BOXONE, BOXTWO, BOXTHREE, BOXFOUR.
– Meini
Nov 16 '18 at 13:29
What you mean by go back? If you create an instance of a subclass doesn't mean u create two instances, so there is no shared screenElements at all.
– Meini
Nov 16 '18 at 13:39
I refactored my code and it works like a charm! Thank you very much!
– MWB
Nov 16 '18 at 13:54
add a comment |
This should work:
public class BasicLayout {
protected HashMap<String, ScreenElement> screenElements = new HashMap<>();
public BasicLayout()
{
screenElements.put("BACKGROUND", new ScreenElement(...));
screenElements.put("BOXONE", new ScreenElement(...));
screenElements.put("BOXTWO", new ScreenElement(...));
}
public HashMap<String, ScreenElement> getScreenElements() {
return screenElements;
}
public ScreenElement getScreenElement(String elementName) {
return screenElements.get(elementName);
}
public void draw(Canvas canvas) {
for (ScreenElement screenElement: screenElements) {
screenElement.draw(canvas);
}
}
}
public class OtherLayout extends BasicLayout {
public OtherLayout() {
screenElements.put("BOXTHREE", new ScreenElement(...));
screenElements.put("BOXFOUR", new ScreenElement(...));
}
}
This should work:
public class BasicLayout {
protected HashMap<String, ScreenElement> screenElements = new HashMap<>();
public BasicLayout()
{
screenElements.put("BACKGROUND", new ScreenElement(...));
screenElements.put("BOXONE", new ScreenElement(...));
screenElements.put("BOXTWO", new ScreenElement(...));
}
public HashMap<String, ScreenElement> getScreenElements() {
return screenElements;
}
public ScreenElement getScreenElement(String elementName) {
return screenElements.get(elementName);
}
public void draw(Canvas canvas) {
for (ScreenElement screenElement: screenElements) {
screenElement.draw(canvas);
}
}
}
public class OtherLayout extends BasicLayout {
public OtherLayout() {
screenElements.put("BOXTHREE", new ScreenElement(...));
screenElements.put("BOXFOUR", new ScreenElement(...));
}
}
edited Nov 16 '18 at 13:26
answered Nov 16 '18 at 13:21
MeiniMeini
414114
414114
If you create a BasicLayout u have BACKGROUND, BOXONE and BOXTWO, if you create a OtherLayout you have BACKGROUND, BOXONE, BOXTWO, BOXTHREE, BOXFOUR.
– Meini
Nov 16 '18 at 13:29
What you mean by go back? If you create an instance of a subclass doesn't mean u create two instances, so there is no shared screenElements at all.
– Meini
Nov 16 '18 at 13:39
I refactored my code and it works like a charm! Thank you very much!
– MWB
Nov 16 '18 at 13:54
add a comment |
If you create a BasicLayout u have BACKGROUND, BOXONE and BOXTWO, if you create a OtherLayout you have BACKGROUND, BOXONE, BOXTWO, BOXTHREE, BOXFOUR.
– Meini
Nov 16 '18 at 13:29
What you mean by go back? If you create an instance of a subclass doesn't mean u create two instances, so there is no shared screenElements at all.
– Meini
Nov 16 '18 at 13:39
I refactored my code and it works like a charm! Thank you very much!
– MWB
Nov 16 '18 at 13:54
If you create a BasicLayout u have BACKGROUND, BOXONE and BOXTWO, if you create a OtherLayout you have BACKGROUND, BOXONE, BOXTWO, BOXTHREE, BOXFOUR.
– Meini
Nov 16 '18 at 13:29
If you create a BasicLayout u have BACKGROUND, BOXONE and BOXTWO, if you create a OtherLayout you have BACKGROUND, BOXONE, BOXTWO, BOXTHREE, BOXFOUR.
– Meini
Nov 16 '18 at 13:29
What you mean by go back? If you create an instance of a subclass doesn't mean u create two instances, so there is no shared screenElements at all.
– Meini
Nov 16 '18 at 13:39
What you mean by go back? If you create an instance of a subclass doesn't mean u create two instances, so there is no shared screenElements at all.
– Meini
Nov 16 '18 at 13:39
I refactored my code and it works like a charm! Thank you very much!
– MWB
Nov 16 '18 at 13:54
I refactored my code and it works like a charm! Thank you very much!
– MWB
Nov 16 '18 at 13:54
add a comment |
I don't think solving inheritance is a good choice for avoiding duplication. You always violate Liskov substitution principle is some settle way (in your case not so settle:)). And that leads to far bigger problems down the line.
And the second point inheritance should be used only to share behaviour not data.
I think in your case the delegation/composition is a better fit:
public OtherLayout() {
screenElements = new HashMap<>();
screenElements.putAll(new BasicLayout().getScreenElements());
screenElements.put("BOXTHREE", new ScreenElement());
screenElements.put("BOXFOUR", new ScreenElement());
}
Because from the looks of it the only thing that is common for the two classes are the elements in the source elements map.
And a more general note. Don't be afraid of duplication as much as the wrong abstraction. The quote from the pioneer of modern computer science (Edsger Dijkstra):
The purpose of abstraction is not to be vague, but to create a new
semantic level in which one can be absolutely precise
You are right, I am primarily sharing data (and some behavior). In your solution, each different type of layout (I have not onlyOtherLayout
but alsoOtherLayout2
,OtherLayout3
,...) needs its owngetters
anddraw
methods, right? You don't see that as a problem? Especially if you have many layouts? Meini's answer works nicely and the code is nice and concise.
– MWB
Nov 16 '18 at 14:21
The solution might look good in terms of numbers of lines but to me the intention is not clear. I like to look at the code and know what's going on immediately. That is why I said that inheritance is not to share data. Suddenly you have to follow this fuzzy logic of state mutation that now spans 2 classes. And the tree of class hierarchy tends to get bigger, with time, not smaller.
– piotr szybicki
Nov 16 '18 at 15:18
The solution proposed by @Meini is very clever. But in my opinion it is too clever for it's own good :)
– piotr szybicki
Nov 16 '18 at 15:21
add a comment |
I don't think solving inheritance is a good choice for avoiding duplication. You always violate Liskov substitution principle is some settle way (in your case not so settle:)). And that leads to far bigger problems down the line.
And the second point inheritance should be used only to share behaviour not data.
I think in your case the delegation/composition is a better fit:
public OtherLayout() {
screenElements = new HashMap<>();
screenElements.putAll(new BasicLayout().getScreenElements());
screenElements.put("BOXTHREE", new ScreenElement());
screenElements.put("BOXFOUR", new ScreenElement());
}
Because from the looks of it the only thing that is common for the two classes are the elements in the source elements map.
And a more general note. Don't be afraid of duplication as much as the wrong abstraction. The quote from the pioneer of modern computer science (Edsger Dijkstra):
The purpose of abstraction is not to be vague, but to create a new
semantic level in which one can be absolutely precise
You are right, I am primarily sharing data (and some behavior). In your solution, each different type of layout (I have not onlyOtherLayout
but alsoOtherLayout2
,OtherLayout3
,...) needs its owngetters
anddraw
methods, right? You don't see that as a problem? Especially if you have many layouts? Meini's answer works nicely and the code is nice and concise.
– MWB
Nov 16 '18 at 14:21
The solution might look good in terms of numbers of lines but to me the intention is not clear. I like to look at the code and know what's going on immediately. That is why I said that inheritance is not to share data. Suddenly you have to follow this fuzzy logic of state mutation that now spans 2 classes. And the tree of class hierarchy tends to get bigger, with time, not smaller.
– piotr szybicki
Nov 16 '18 at 15:18
The solution proposed by @Meini is very clever. But in my opinion it is too clever for it's own good :)
– piotr szybicki
Nov 16 '18 at 15:21
add a comment |
I don't think solving inheritance is a good choice for avoiding duplication. You always violate Liskov substitution principle is some settle way (in your case not so settle:)). And that leads to far bigger problems down the line.
And the second point inheritance should be used only to share behaviour not data.
I think in your case the delegation/composition is a better fit:
public OtherLayout() {
screenElements = new HashMap<>();
screenElements.putAll(new BasicLayout().getScreenElements());
screenElements.put("BOXTHREE", new ScreenElement());
screenElements.put("BOXFOUR", new ScreenElement());
}
Because from the looks of it the only thing that is common for the two classes are the elements in the source elements map.
And a more general note. Don't be afraid of duplication as much as the wrong abstraction. The quote from the pioneer of modern computer science (Edsger Dijkstra):
The purpose of abstraction is not to be vague, but to create a new
semantic level in which one can be absolutely precise
I don't think solving inheritance is a good choice for avoiding duplication. You always violate Liskov substitution principle is some settle way (in your case not so settle:)). And that leads to far bigger problems down the line.
And the second point inheritance should be used only to share behaviour not data.
I think in your case the delegation/composition is a better fit:
public OtherLayout() {
screenElements = new HashMap<>();
screenElements.putAll(new BasicLayout().getScreenElements());
screenElements.put("BOXTHREE", new ScreenElement());
screenElements.put("BOXFOUR", new ScreenElement());
}
Because from the looks of it the only thing that is common for the two classes are the elements in the source elements map.
And a more general note. Don't be afraid of duplication as much as the wrong abstraction. The quote from the pioneer of modern computer science (Edsger Dijkstra):
The purpose of abstraction is not to be vague, but to create a new
semantic level in which one can be absolutely precise
answered Nov 16 '18 at 14:00
piotr szybickipiotr szybicki
7011511
7011511
You are right, I am primarily sharing data (and some behavior). In your solution, each different type of layout (I have not onlyOtherLayout
but alsoOtherLayout2
,OtherLayout3
,...) needs its owngetters
anddraw
methods, right? You don't see that as a problem? Especially if you have many layouts? Meini's answer works nicely and the code is nice and concise.
– MWB
Nov 16 '18 at 14:21
The solution might look good in terms of numbers of lines but to me the intention is not clear. I like to look at the code and know what's going on immediately. That is why I said that inheritance is not to share data. Suddenly you have to follow this fuzzy logic of state mutation that now spans 2 classes. And the tree of class hierarchy tends to get bigger, with time, not smaller.
– piotr szybicki
Nov 16 '18 at 15:18
The solution proposed by @Meini is very clever. But in my opinion it is too clever for it's own good :)
– piotr szybicki
Nov 16 '18 at 15:21
add a comment |
You are right, I am primarily sharing data (and some behavior). In your solution, each different type of layout (I have not onlyOtherLayout
but alsoOtherLayout2
,OtherLayout3
,...) needs its owngetters
anddraw
methods, right? You don't see that as a problem? Especially if you have many layouts? Meini's answer works nicely and the code is nice and concise.
– MWB
Nov 16 '18 at 14:21
The solution might look good in terms of numbers of lines but to me the intention is not clear. I like to look at the code and know what's going on immediately. That is why I said that inheritance is not to share data. Suddenly you have to follow this fuzzy logic of state mutation that now spans 2 classes. And the tree of class hierarchy tends to get bigger, with time, not smaller.
– piotr szybicki
Nov 16 '18 at 15:18
The solution proposed by @Meini is very clever. But in my opinion it is too clever for it's own good :)
– piotr szybicki
Nov 16 '18 at 15:21
You are right, I am primarily sharing data (and some behavior). In your solution, each different type of layout (I have not only
OtherLayout
but also OtherLayout2
, OtherLayout3
,...) needs its own getters
and draw
methods, right? You don't see that as a problem? Especially if you have many layouts? Meini's answer works nicely and the code is nice and concise.– MWB
Nov 16 '18 at 14:21
You are right, I am primarily sharing data (and some behavior). In your solution, each different type of layout (I have not only
OtherLayout
but also OtherLayout2
, OtherLayout3
,...) needs its own getters
and draw
methods, right? You don't see that as a problem? Especially if you have many layouts? Meini's answer works nicely and the code is nice and concise.– MWB
Nov 16 '18 at 14:21
The solution might look good in terms of numbers of lines but to me the intention is not clear. I like to look at the code and know what's going on immediately. That is why I said that inheritance is not to share data. Suddenly you have to follow this fuzzy logic of state mutation that now spans 2 classes. And the tree of class hierarchy tends to get bigger, with time, not smaller.
– piotr szybicki
Nov 16 '18 at 15:18
The solution might look good in terms of numbers of lines but to me the intention is not clear. I like to look at the code and know what's going on immediately. That is why I said that inheritance is not to share data. Suddenly you have to follow this fuzzy logic of state mutation that now spans 2 classes. And the tree of class hierarchy tends to get bigger, with time, not smaller.
– piotr szybicki
Nov 16 '18 at 15:18
The solution proposed by @Meini is very clever. But in my opinion it is too clever for it's own good :)
– piotr szybicki
Nov 16 '18 at 15:21
The solution proposed by @Meini is very clever. But in my opinion it is too clever for it's own good :)
– piotr szybicki
Nov 16 '18 at 15:21
add a comment |
Based on the sample code, there only needs to be a single class which takes the elements it will be responsible for in the constructor. For example:
public class Layout {
private final Map<String, ScreenElement> screenElements;
public Layout(Map<String, ScreenElement> screenElements) {
this.screenElements = screenElements;
}
// other methods
}
From your example, somewhere in the code base would know to create a BasicLayout
or OtherLayout
. Refactor that code and extract the map, and pass to the constructor. For example:
// in some method that knows when to create a `BasicLayout` or `OtherLayout`
Map<String, ScreenElement> basicScreenElements = new HashMap<>();
basicScreenElements .put("BACKGROUND", new ScreenElement(...));
basicScreenElements .put("BOXONE", new ScreenElement(...));
basicScreenElements .put("BOXTWO", new ScreenElement(...));
Layout basicLayout = new Layout(basicScreenElements);
// ....
Map<String, ScreenElement> otherScreenElements = new HashMap<>();
otherScreenElements .put("BOXTHREE", new ScreenElement(...));
otherScreenElements .put("BOXFOUR", new ScreenElement(...));
Layout otherLayout = new Layout(otherScreenElements);
In your example,otherLayout
does not contain BACKGROUND, BOXONE and BOXTWO. If you want this, you need to add them, which leads to code duplication.
– MWB
Nov 16 '18 at 13:55
The creation of aScreenElement
object could be moved to a factory class, or provide aScreenElement
copy constructor.
– Andrew S
Nov 16 '18 at 14:04
add a comment |
Based on the sample code, there only needs to be a single class which takes the elements it will be responsible for in the constructor. For example:
public class Layout {
private final Map<String, ScreenElement> screenElements;
public Layout(Map<String, ScreenElement> screenElements) {
this.screenElements = screenElements;
}
// other methods
}
From your example, somewhere in the code base would know to create a BasicLayout
or OtherLayout
. Refactor that code and extract the map, and pass to the constructor. For example:
// in some method that knows when to create a `BasicLayout` or `OtherLayout`
Map<String, ScreenElement> basicScreenElements = new HashMap<>();
basicScreenElements .put("BACKGROUND", new ScreenElement(...));
basicScreenElements .put("BOXONE", new ScreenElement(...));
basicScreenElements .put("BOXTWO", new ScreenElement(...));
Layout basicLayout = new Layout(basicScreenElements);
// ....
Map<String, ScreenElement> otherScreenElements = new HashMap<>();
otherScreenElements .put("BOXTHREE", new ScreenElement(...));
otherScreenElements .put("BOXFOUR", new ScreenElement(...));
Layout otherLayout = new Layout(otherScreenElements);
In your example,otherLayout
does not contain BACKGROUND, BOXONE and BOXTWO. If you want this, you need to add them, which leads to code duplication.
– MWB
Nov 16 '18 at 13:55
The creation of aScreenElement
object could be moved to a factory class, or provide aScreenElement
copy constructor.
– Andrew S
Nov 16 '18 at 14:04
add a comment |
Based on the sample code, there only needs to be a single class which takes the elements it will be responsible for in the constructor. For example:
public class Layout {
private final Map<String, ScreenElement> screenElements;
public Layout(Map<String, ScreenElement> screenElements) {
this.screenElements = screenElements;
}
// other methods
}
From your example, somewhere in the code base would know to create a BasicLayout
or OtherLayout
. Refactor that code and extract the map, and pass to the constructor. For example:
// in some method that knows when to create a `BasicLayout` or `OtherLayout`
Map<String, ScreenElement> basicScreenElements = new HashMap<>();
basicScreenElements .put("BACKGROUND", new ScreenElement(...));
basicScreenElements .put("BOXONE", new ScreenElement(...));
basicScreenElements .put("BOXTWO", new ScreenElement(...));
Layout basicLayout = new Layout(basicScreenElements);
// ....
Map<String, ScreenElement> otherScreenElements = new HashMap<>();
otherScreenElements .put("BOXTHREE", new ScreenElement(...));
otherScreenElements .put("BOXFOUR", new ScreenElement(...));
Layout otherLayout = new Layout(otherScreenElements);
Based on the sample code, there only needs to be a single class which takes the elements it will be responsible for in the constructor. For example:
public class Layout {
private final Map<String, ScreenElement> screenElements;
public Layout(Map<String, ScreenElement> screenElements) {
this.screenElements = screenElements;
}
// other methods
}
From your example, somewhere in the code base would know to create a BasicLayout
or OtherLayout
. Refactor that code and extract the map, and pass to the constructor. For example:
// in some method that knows when to create a `BasicLayout` or `OtherLayout`
Map<String, ScreenElement> basicScreenElements = new HashMap<>();
basicScreenElements .put("BACKGROUND", new ScreenElement(...));
basicScreenElements .put("BOXONE", new ScreenElement(...));
basicScreenElements .put("BOXTWO", new ScreenElement(...));
Layout basicLayout = new Layout(basicScreenElements);
// ....
Map<String, ScreenElement> otherScreenElements = new HashMap<>();
otherScreenElements .put("BOXTHREE", new ScreenElement(...));
otherScreenElements .put("BOXFOUR", new ScreenElement(...));
Layout otherLayout = new Layout(otherScreenElements);
answered Nov 16 '18 at 13:35
Andrew SAndrew S
1,5911511
1,5911511
In your example,otherLayout
does not contain BACKGROUND, BOXONE and BOXTWO. If you want this, you need to add them, which leads to code duplication.
– MWB
Nov 16 '18 at 13:55
The creation of aScreenElement
object could be moved to a factory class, or provide aScreenElement
copy constructor.
– Andrew S
Nov 16 '18 at 14:04
add a comment |
In your example,otherLayout
does not contain BACKGROUND, BOXONE and BOXTWO. If you want this, you need to add them, which leads to code duplication.
– MWB
Nov 16 '18 at 13:55
The creation of aScreenElement
object could be moved to a factory class, or provide aScreenElement
copy constructor.
– Andrew S
Nov 16 '18 at 14:04
In your example,
otherLayout
does not contain BACKGROUND, BOXONE and BOXTWO. If you want this, you need to add them, which leads to code duplication.– MWB
Nov 16 '18 at 13:55
In your example,
otherLayout
does not contain BACKGROUND, BOXONE and BOXTWO. If you want this, you need to add them, which leads to code duplication.– MWB
Nov 16 '18 at 13:55
The creation of a
ScreenElement
object could be moved to a factory class, or provide a ScreenElement
copy constructor.– Andrew S
Nov 16 '18 at 14:04
The creation of a
ScreenElement
object could be moved to a factory class, or provide a ScreenElement
copy constructor.– Andrew S
Nov 16 '18 at 14:04
add a comment |
Thanks for contributing an answer to Stack Overflow!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fstackoverflow.com%2fquestions%2f53338543%2fhow-to-solve-code-duplication-in-this-example-where-i-introduce-inheritance-to-a%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown