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;
}







0















Suppose I have an App with different Scenes. Each Scene has a layout that is build up by a number of ScreenElements, 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 ScreenElements in a HashMap (or ultimately an EnumMap) so that I can call on the ScreenElements 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.










share|improve this question































    0















    Suppose I have an App with different Scenes. Each Scene has a layout that is build up by a number of ScreenElements, 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 ScreenElements in a HashMap (or ultimately an EnumMap) so that I can call on the ScreenElements 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.










    share|improve this question



























      0












      0








      0








      Suppose I have an App with different Scenes. Each Scene has a layout that is build up by a number of ScreenElements, 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 ScreenElements in a HashMap (or ultimately an EnumMap) so that I can call on the ScreenElements 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.










      share|improve this question
















      Suppose I have an App with different Scenes. Each Scene has a layout that is build up by a number of ScreenElements, 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 ScreenElements in a HashMap (or ultimately an EnumMap) so that I can call on the ScreenElements 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






      share|improve this question















      share|improve this question













      share|improve this question




      share|improve this question








      edited Nov 16 '18 at 14:28







      MWB

















      asked Nov 16 '18 at 13:07









      MWBMWB

      9831819




      9831819
























          3 Answers
          3






          active

          oldest

          votes


















          1














          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(...));
          }
          }





          share|improve this answer


























          • 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



















          1














          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







          share|improve this answer
























          • 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 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



















          0














          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);





          share|improve this answer
























          • 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














          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
          });


          }
          });














          draft saved

          draft discarded


















          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









          1














          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(...));
          }
          }





          share|improve this answer


























          • 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
















          1














          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(...));
          }
          }





          share|improve this answer


























          • 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














          1












          1








          1







          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(...));
          }
          }





          share|improve this answer















          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(...));
          }
          }






          share|improve this answer














          share|improve this answer



          share|improve this answer








          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



















          • 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













          1














          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







          share|improve this answer
























          • 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 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
















          1














          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







          share|improve this answer
























          • 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 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














          1












          1








          1







          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







          share|improve this answer













          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








          share|improve this answer












          share|improve this answer



          share|improve this answer










          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 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 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













          • 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











          0














          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);





          share|improve this answer
























          • 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


















          0














          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);





          share|improve this answer
























          • 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
















          0












          0








          0







          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);





          share|improve this answer













          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);






          share|improve this answer












          share|improve this answer



          share|improve this answer










          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 a ScreenElement object could be moved to a factory class, or provide a ScreenElement 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











          • 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



















          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




















          draft saved

          draft discarded




















































          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.




          draft saved


          draft discarded














          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





















































          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







          Popular posts from this blog

          Florida Star v. B. J. F.

          Danny Elfman

          Retrieve a Users Dashboard in Tumblr with R and TumblR. Oauth Issues