Reputation: 1
It's been about 20 years since I have taken a coding class. I pick it up again from time to time for fun, but my code is clunky & inefficient.
I have an array with just over 400 elements. I currently have nested foreach loops operating over that array.
import acm.program.ConsoleProgram;
import java.awt.Color;
import acm.io.IODialog;
import java.text.*;
import static java.lang.Math.*;
import java.util.*;
/** Tests to see if user color matches sample colors */
public class test extends ConsoleProgram
{
//defining sample colors
Color[] dmc =
{
new Color(255,255,255),
new Color(148,91,128),
new Color(206,148,186),
new Color(236,207,225),
new Color(243,218,228),
};
public void run()
{
average();
}
//averages three colors, then tests for match to given color
public void average()
{
//asks for user color
IODialog dialog = new IODialog();
int stitchRed= dialog.readInt("Enter red value: ");
int stitchGreen= dialog.readInt("Enter green value: ");
int stitchBlue= dialog.readInt("Enter blue value: ");
Color stitchColor= new Color(stitchRed,stitchGreen,stitchBlue);
//gets averages for dmc colors
for (Color i:dmc)
{
for (Color j:dmc)
{
for (Color k:dmc)
{
int indexI = Arrays.asList(dmc).indexOf(i);
int indexJ = Arrays.asList(dmc).indexOf(j);
int indexK = Arrays.asList(dmc).indexOf(k);
if(indexI <= indexJ && indexJ <= indexK)
{
int iRed = i.getRed();
int jRed = j.getRed();
int kRed = k.getRed();
int iGreen = i.getGreen();
int jGreen = j.getGreen();
int kGreen = k.getGreen();
int iBlue = i.getBlue();
int jBlue = j.getBlue();
int kBlue = k.getBlue();
int redAverage = (iRed+jRed+kRed)/3;
int greenAverage = (iGreen+jGreen+kGreen)/3;
int blueAverage = (iBlue+jBlue+kBlue)/3;
Color colorAverage = new Color(redAverage,greenAverage,blueAverage);
//tests to see if any thread average equals user color
if (colorAverage.equals(stitchColor))
{
println("The color match is: " + i + ", " + j + ", " + k);
}
}
}
}
}
println("no more matches");
}
}
This compiles fine, but runs REALLY slowly.
Is there a more efficient way to do this?
Possibly a way to get around nesting - something to the effect of:
for( Color i,j,k:dmc)
?
Upvotes: 0
Views: 1237
Reputation: 18812
(I can't test and debug the code since it is not an SSCCE, so I appologies for any errors)
Consider the following, see comments for details:
Color[ ] dmc = {element1, element2, ...};
for( int i =0; i < dmc.length ; i++ )
{
//iterate only on values >= i
for( int j = i; j< dmc.length; j++ )
{
//loop only on values >= j
for( int k = j; k < dmc.length ; k++ )
{
//not needed, you already have i, j, k
//int indexI = Arrays.asList(dmc).indexOf(i);
//int indexJ = Arrays.asList(dmc).indexOf(j);
//int indexK = Arrays.asList(dmc).indexOf(k);
//condition is not needed. Allways true.
//if( indexI <= indexJ && indexJ <= indexK ) { }
//bunch of code...
EDIT: A short demo to print out only the wanted incidences:
public static void main(String[] args) {
Color[ ] dmc = {Color.AQUA, Color.AZURE, Color.BEIGE, Color.BLACK, Color.BLUE,
Color.BROWN, Color.CHOCOLATE, Color.CYAN, Color.CRIMSON, Color.FUCHSIA};
for( int i =0; i < dmc.length ; i++ )
{
System.out.printf("\n >>>>>> i= %s", i);
//iterate only on values >= i
for( int j = i; j< dmc.length; j++ )
{
System.out.printf("\n j= %s",j);
//loop only on values >= j
for( int k = j; k < dmc.length ; k++ )
{
System.out.printf("\n k= %s",k);
}
}
}
}
EDIT II: Following the MCVE posted I modified it by adding fastEverage()
method, which should do the same calculation as everage()
with less iterations.
On the single set of colors I checked, both methods have the same output:
import java.awt.Color;
import java.util.Arrays;
/** Tests to see if user color matches sample colors */
public class Test
{
//defining sample colors
Color[] dmc =
{
new Color(255,255,255),
new Color(148,91,128),
new Color(206,148,186),
new Color(236,207,225),
new Color(243,218,228),
};
//averages three colors, then tests for match to given color
public void average()
{
//asks for user color
int stitchRed = 203;
int stitchGreen= 164;
int stitchBlue= 189;
Color stitchColor= new Color(stitchRed,stitchGreen,stitchBlue);
//gets averages for dmc colors
for (Color i:dmc)
{
for (Color j:dmc)
{
for (Color k:dmc)
{
int indexI = Arrays.asList(dmc).indexOf(i);
int indexJ = Arrays.asList(dmc).indexOf(j);
int indexK = Arrays.asList(dmc).indexOf(k);
if((indexI <= indexJ) && (indexJ <= indexK))
{
int iRed = i.getRed();
int jRed = j.getRed();
int kRed = k.getRed();
int iGreen = i.getGreen();
int kGreen = k.getGreen();
int jGreen = j.getGreen();
int iBlue = i.getBlue();
int jBlue = j.getBlue();
int kBlue = k.getBlue();
int redAverage = (iRed+jRed+kRed)/3;
int greenAverage = (iGreen+ jGreen + kGreen)/3;
int blueAverage = (iBlue+jBlue+kBlue)/3;
Color colorAverage = new Color(redAverage,greenAverage,blueAverage);
//tests to see if any thread average equals user color
if (colorAverage.equals(stitchColor))
{
System.out.println("The color match is: " + i + ", " + j + ", " + k);
}
}
}
}
}
System.out.println("no more matches");
}
//averages three colors, then tests for match to given color
public void fastEverage()
{
//asks for user color
int stitchRed = 203;
int stitchGreen= 164;
int stitchBlue= 189;
Color stitchColor= new Color(stitchRed,stitchGreen,stitchBlue);
Color colorI, colorJ, colorK;
for( int i =0; i < dmc.length ; i++ )
{
colorI = dmc[i];
//iterate only on values >= i
for( int j = i; j< dmc.length; j++ )
{
colorJ = dmc[j];
//loop only on values >= j
for( int k = j; k < dmc.length ; k++ )
{
colorK = dmc[k];
int iRed = colorI.getRed();
int jRed = colorJ.getRed();
int kRed = colorK.getRed();
int iGreen = colorI.getGreen();
int kGreen = colorK.getGreen();
int jGreen = colorJ.getGreen();
int iBlue = colorI.getBlue();
int jBlue = colorJ.getBlue();
int kBlue = colorK.getBlue();
int redAverage = (iRed+jRed+kRed)/3;
int greenAverage = (iGreen+ jGreen + kGreen)/3;
int blueAverage = (iBlue+jBlue+kBlue)/3;
Color colorAverage = new Color(redAverage,greenAverage,blueAverage);
//tests to see if any thread average equals user color
if (colorAverage.equals(stitchColor))
{
System.out.println("The color match is: " + colorI + ", " + colorJ + ", " + colorK);
}
}
}
}
System.out.println("no more matches");
}
public static void main(String[] args)
{
new Test().average();
new Test().fastEverage();
}
}
The run time difference between the 2 methods, on this test case, is significant:
run time of everage() in nano seconds 84752814
run time of fastEverage() in nano seconds 76497255
Upvotes: 1
Reputation: 5068
It's equals to @C0der's answer, Just uses the same names for question's var.
for (int indexI = 0; indexI < dmc.length; indexI++) {
Color i = dmc[indexI];
for (int indexJ = indexI; indexJ< dmc.length; indexJ++) {
Color j = dmc[indexJ];
for (int indexK = indexJ; indexK< dmc.length; indexK++) {
Color k = dmc[indexK];
//....
}
}
}
EDITED:
public void average() {
//asks for user color
IODialog dialog = new IODialog();
int stitchRed = dialog.readInt("Enter red value: ");
int stitchGreen = dialog.readInt("Enter green value: ");
int stitchBlue = dialog.readInt("Enter blue value: ");
Color stitchColor = new Color(stitchRed, stitchGreen, stitchBlue);
for (int indexI = 0; indexI < dmc.length; indexI++) {
Color i = dmc[indexI];
for (int indexJ = indexI; indexJ < dmc.length; indexJ++) {
Color j = dmc[indexJ];
for (int indexK = indexJ; indexK < dmc.length; indexK++) {
Color k = dmc[indexK];
int iRed = i.getRed();
int jRed = j.getRed();
int kRed = k.getRed();
int iGreen = i.getGreen();
int jGreen = j.getGreen();
int kGreen = k.getGreen();
int iBlue = i.getBlue();
int jBlue = j.getBlue();
int kBlue = k.getBlue();
int redAverage = (iRed + jRed + kRed) / 3;
int greenAverage = (iGreen + jGreen + kGreen) / 3;
int blueAverage = (iBlue + jBlue + kBlue) / 3;
Color colorAverage = new Color(redAverage, greenAverage, blueAverage);
//tests to see if any thread average equals user color
if (colorAverage.equals(stitchColor)) {
System.out.println("The color match is: " + i + ", " + j + ", " + k);
}
}
}
}
System.out.println("no more matches");
}
Upvotes: 1
Reputation: 184
Just an idea. You can split the search. There is a possibility to find the match within the first search. If so, then you don't need to search the rest for a match.If not, you can move forward to search for the match.
int i,j,k;
for(i=0;i<100;i++){
for(j=i;j<100;j++){
for(k=j;k<100;k++){
//..do stuffs.....//
}
}
}
if(matchnotfound){
for(i=100;i<200;i++){
for(j=i;j<200;j++){
for(k=j;k<200;k++){
//..do stuffs.....//
}
}
}
}
if(matchnotfound){
for(i=200;i<300;i++){
for(j=i;j<300;j++){
for(k=j;k<300;k++){
//..do stuffs.....//
}
}
}
}
if(matchnotfound){
for(i=300;i<400;i++){
for(j=i;j<400;j++){
for(k=j;k<400;k++){
//..do stuffs.....//
}
}
}
}
Upvotes: 0
Reputation: 184
You can try something like
for( x=1;x<401;x++ )
{
Color j= dmc[x];
Color k= dmc[x+1];
int indexI = Arrays.asList(dmc).indexOf(i);
int indexJ = Arrays.asList(dmc).indexOf(j);
int indexK = Arrays.asList(dmc).indexOf(k);
if( indexI <= indexJ && indexJ <= indexK )
{
//bunch of code...
or, you can try equals method defined in java.lang.Object. Hope this will help.
Upvotes: 0
Reputation: 136
Considering you filter out many combinations in the if-statement, one optimization would be to use for-loops only iterating over the elements you actually want to process instead of for-each-loops. E.g. i <= j means you can start the iteration in j at i.
Further improvements may very well be possible depending on your intentions as stated in the comments.
Upvotes: 0