Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

##Arrays and loops are your friend.

Arrays and loops are your friend.

g2.draw(lin1);
g2.draw(lin2);
g2.draw(lin3);
g2.draw(lin4);
g2.draw(lin5);
g2.draw(lin6);
g2.draw(lin7);
g2.draw(lin8);
g2.draw(lin9);
g2.draw(lin10);
g2.draw(lin11);
g2.draw(lin12);
g2.draw(lin13);
g2.draw(lin14);
g2.draw(lin15);
g2.draw(lin16);
g2.draw(lin17);
g2.draw(lin18);
g2.draw(lin19);
g2.draw(lin20);
g2.draw(lin21);
g2.draw(lin22);
g2.draw(lin23);

If you held all these Line2D as an array named lines could refactor the above as:

for (Line2D line : lines) {
 g2.draw(line);
}

Similarly, if you declare your Point2DFloat as an array you could simply match the right points using index e.g.

pointfloats[0] = new Point2D.Float(245, 88);
 // etc...
for (int i = 0, j = 0; i < lines.length; i++, j += 2) {
 lines[i] = new Line2D.Float(pointfloats[j], pointfloats[j + 1]);
}

Do note that we're still declaring the points line by line. This is undesirable and you have 'magic numbers' all over the place. A better approach would be to externalize this data, and read it all into a data array so you can just have 3 arrays and loops to connect them. This also has the added benefit of not needing to compile for a data change.

This really goes for most of this code, as all that needs to be done is to hold the objects in an array e.g. your ActionListener setSelected procedure should just be 1 loop.

##Arrays and loops are your friend.

g2.draw(lin1);
g2.draw(lin2);
g2.draw(lin3);
g2.draw(lin4);
g2.draw(lin5);
g2.draw(lin6);
g2.draw(lin7);
g2.draw(lin8);
g2.draw(lin9);
g2.draw(lin10);
g2.draw(lin11);
g2.draw(lin12);
g2.draw(lin13);
g2.draw(lin14);
g2.draw(lin15);
g2.draw(lin16);
g2.draw(lin17);
g2.draw(lin18);
g2.draw(lin19);
g2.draw(lin20);
g2.draw(lin21);
g2.draw(lin22);
g2.draw(lin23);

If you held all these Line2D as an array named lines could refactor the above as:

for (Line2D line : lines) {
 g2.draw(line);
}

Similarly, if you declare your Point2DFloat as an array you could simply match the right points using index e.g.

pointfloats[0] = new Point2D.Float(245, 88);
 // etc...
for (int i = 0, j = 0; i < lines.length; i++, j += 2) {
 lines[i] = new Line2D.Float(pointfloats[j], pointfloats[j + 1]);
}

Do note that we're still declaring the points line by line. This is undesirable and you have 'magic numbers' all over the place. A better approach would be to externalize this data, and read it all into a data array so you can just have 3 arrays and loops to connect them. This also has the added benefit of not needing to compile for a data change.

This really goes for most of this code, as all that needs to be done is to hold the objects in an array e.g. your ActionListener setSelected procedure should just be 1 loop.

Arrays and loops are your friend.

g2.draw(lin1);
g2.draw(lin2);
g2.draw(lin3);
g2.draw(lin4);
g2.draw(lin5);
g2.draw(lin6);
g2.draw(lin7);
g2.draw(lin8);
g2.draw(lin9);
g2.draw(lin10);
g2.draw(lin11);
g2.draw(lin12);
g2.draw(lin13);
g2.draw(lin14);
g2.draw(lin15);
g2.draw(lin16);
g2.draw(lin17);
g2.draw(lin18);
g2.draw(lin19);
g2.draw(lin20);
g2.draw(lin21);
g2.draw(lin22);
g2.draw(lin23);

If you held all these Line2D as an array named lines could refactor the above as:

for (Line2D line : lines) {
 g2.draw(line);
}

Similarly, if you declare your Point2DFloat as an array you could simply match the right points using index e.g.

pointfloats[0] = new Point2D.Float(245, 88);
 // etc...
for (int i = 0, j = 0; i < lines.length; i++, j += 2) {
 lines[i] = new Line2D.Float(pointfloats[j], pointfloats[j + 1]);
}

Do note that we're still declaring the points line by line. This is undesirable and you have 'magic numbers' all over the place. A better approach would be to externalize this data, and read it all into a data array so you can just have 3 arrays and loops to connect them. This also has the added benefit of not needing to compile for a data change.

This really goes for most of this code, as all that needs to be done is to hold the objects in an array e.g. your ActionListener setSelected procedure should just be 1 loop.

Source Link
Legato
  • 9.9k
  • 4
  • 50
  • 118

##Arrays and loops are your friend.

g2.draw(lin1);
g2.draw(lin2);
g2.draw(lin3);
g2.draw(lin4);
g2.draw(lin5);
g2.draw(lin6);
g2.draw(lin7);
g2.draw(lin8);
g2.draw(lin9);
g2.draw(lin10);
g2.draw(lin11);
g2.draw(lin12);
g2.draw(lin13);
g2.draw(lin14);
g2.draw(lin15);
g2.draw(lin16);
g2.draw(lin17);
g2.draw(lin18);
g2.draw(lin19);
g2.draw(lin20);
g2.draw(lin21);
g2.draw(lin22);
g2.draw(lin23);

If you held all these Line2D as an array named lines could refactor the above as:

for (Line2D line : lines) {
 g2.draw(line);
}

Similarly, if you declare your Point2DFloat as an array you could simply match the right points using index e.g.

pointfloats[0] = new Point2D.Float(245, 88);
 // etc...
for (int i = 0, j = 0; i < lines.length; i++, j += 2) {
 lines[i] = new Line2D.Float(pointfloats[j], pointfloats[j + 1]);
}

Do note that we're still declaring the points line by line. This is undesirable and you have 'magic numbers' all over the place. A better approach would be to externalize this data, and read it all into a data array so you can just have 3 arrays and loops to connect them. This also has the added benefit of not needing to compile for a data change.

This really goes for most of this code, as all that needs to be done is to hold the objects in an array e.g. your ActionListener setSelected procedure should just be 1 loop.

lang-java

AltStyle によって変換されたページ (->オリジナル) /