Skip to main content
I went through this answer and fixed what I found, but I forgot what I found so I can't really type a proper edit summary =/
Source Link
Pimgd
  • 22.3k
  • 5
  • 66
  • 144

I'd expect this to be your class-level javadoc. Why is it not? it'sIt's effectively dead, because no IDE or javadoc-tool will pick this up. Integrate it into the already existing javadoc.

alsoAlso, why are you using System.out.println() in a production class?

I'd expect this to be your class-level javadoc. Why is it not? it's effectively dead, because no IDE or javadoc-tool will pick this up. Integrate it into the already existing javadoc.

also why are you using System.out.println() in a production class?

I'd expect this to be your class-level javadoc. Why is it not? It's effectively dead, because no IDE or javadoc-tool will pick this up. Integrate it into the already existing javadoc.

Also, why are you using System.out.println() in a production class?

Source Link
Vogel612
  • 25.3k
  • 7
  • 58
  • 141

Just going through this from top to bottom:

/*
 * A class for resizing Swing components within a container
 * to maintain aspect ratio with the container.
 */

I'd expect this to be your class-level javadoc. Why is it not? it's effectively dead, because no IDE or javadoc-tool will pick this up. Integrate it into the already existing javadoc.


public SwingResizer()
{
  componentResizeMap = new HashMap<>();
  buttonList = new HashSet<>();
  labelList = new HashSet<>();
  textList = new HashSet<>();
  panelList = new HashSet<>();
  lPaneList = new HashSet<>();
}

Why not initialize these when you're declaring them? It's basically faster ... Also I'm not quite sure I want my layout-manager to differentiate between different Component types. We'll see :)

Also: Java convention usually entails "Egyptian braces". Get the first curly one line higher.


 private float convertVectorToScale(int compVec, int contSize)  
 {

Argument names (just like variable names) don't need to be shorter than 8 characters anymore. componentVector and containerSize are clearer in your intent.

     float result = (float) contSize/compVec;

Binary operators are usually surrounded by spaces to ease spotting and processing them:

float result = (float) containerSize / componentVector;

{
  String name = comp.getName();
  System.out.println(comp);

okay... you're grabbing the component's name. At this point I was getting curious, or rather confused. Why would a layout manager need a component's name?

also why are you using System.out.println() in a production class?

And why in gods name do you rely on names?. This also explains why you'd split up your components by type. Actually... it doesn't. That step is breaking things for you. Because you're doing the exact same steps for any given component added:

public void addComponent(Component comp, int x, int y, 
      int xComponentSize, int yComponentSize,
      int xContainerSize, int yContainerSize) {
    float[] scales = new float[] { convertVectorToScale(x, xContainerSize),
        convertVectorToScale(y, yContainerSize),
        convertVectorToScale(xComponentSize, xContainerSize),
        convertVectorToScale(yComponentSize, yContainerSize) };
    componentResizeMap.put(component, scales);
}

^^ this is all the code you need to properly resize the components you get. But for the sake of argument, let's keep reviewing, because there's some things that you still need to hear lest you make the same mistakes in a team:


 if (duplicateKey != null)
 {
   System.out.println("Error - Duplicate component name");
 }

THIS IS DANGEROUS!
Your code recognized there is an error and then proceeds to write a meaningless string out into the console (which nobody ever checks, trust me) and then does business as usual.

The caller of the method has no indication that the method failed in any way. And that's dangerous. Really extremely dangerous. Because someday someone (maybe future you) will want to use this class, oblivious of the fact that the components need distinct names to work right. And stuff will break and it will take hours if not days to find out why?


public void resizeComponents(int x,int y)
{
  try
  {     
   SwingUtilities.invokeLater(new Runnable(){

    public void run()
    {    

     for (JButton button : buttonList)
     {
      try
      {

^^ this ... I ... no. Just no. Following steps:

  1. Get a proper IDE
  2. Use the "indent" feature of your IDE
  3. Look at the lipstick on a pig and then think hard about why you need 5 levels of indentation in one method.

But hey. Aren't we lucky that this can all be much simpler with the rewrite for addComponent?
Look:

public void resizeComponents(int x, int y) {
    SwingUtilities.invokeLater(() -> {
        for (Map.Entry<Component, float[]> componentData : componentResizeMap.entrySet()) {
            float[] aspectRatios = entry.getValue();
            entry.getKey().setBounds(Math.round(x / aspectRatios[0]), 
                      Math.round(y / aspectRatios[1]), 
                      Math.round(x / aspectRatios[2]), 
                      Math.round(y / aspectRatios[3]));
        }
    };
}

Note that this uses a lambda-expression from Java 8 as Runnable instance and it could be simplified quite a bit by using further Java 8 features, such as streaming the entry set.