4
\$\begingroup\$

I'm porting this code to Java and after a few tests all seems ok but I need to know if I'm doing this right.

The code takes some colors and create a color ramp for maps and other stuff. You can see the original Javascript version working here.

I removed the "slider" part and fixed the amplitude to 0..100 for color ramp and 0..1 for levels.

package br.com.cmabreu.color;

import java.awt.Color;
import java.util.ArrayList;
import java.util.List;

public class ColorGradientGenerator {
    private List<ColorItem> gradient;
    
    public ColorGradientGenerator() {
        this.gradient = new ArrayList<ColorItem>();
    }
    
    public void addToGradient( int index, int red, int green, int blue, int alpha ) throws Exception {
        if( index > 100 ) throw new Exception("Index must be <= 100");
        if( index < 0 )   throw new Exception("Index must be >= 0");
        Color color = new Color(red,green,blue,alpha);
        gradient.add( new ColorItem( index, color ) );
    }

    
    public void removeFromGradient( int index ) {
        gradient.remove(index);
    }
    
    private ColorItem getItemZero() throws Exception {
        for ( ColorItem colorItem : gradient  ) {
            if( colorItem.getIndex() == 0 ) return colorItem;
        }
        throw new Exception("Cannot find lower bound color");
    }
    
    
    public Color getColorAt( double value ) throws Exception {
        ColorItem firstColor = null, secondColor = null;
        double firstColorX, secondColorX;
        value = value * 100;
        int iVal = (int)value;
        
        firstColor = getItemZero();
        for ( ColorItem colorItem : gradient  ) {
            if( iVal <= colorItem.getIndex() ) {
                secondColor = colorItem;
                break;
            } else {
                firstColor = colorItem;
            }
        }
        if( secondColor == null ) throw new Exception("Cannot find upper bound color");
        
        firstColorX =  firstColor.getIndex();
        secondColorX = secondColor.getIndex() - firstColorX;
        
        double sliderX = (double)iVal - (double)firstColorX;
        
        double ratio = 0.0;
        if( secondColor.getIndex() > 0) {
            ratio = sliderX / (double)secondColorX;
        }  
        // System.out.println("Sx: " + sliderX + "   Fc: " + firstColor.getIndex() + "  Sc: " + secondColor.getIndex() + "  Ratio: " + ratio );     
        return pickHex( firstColor.getColor(), secondColor.getColor(), ratio);
    }

    
    private Color pickHex( Color firstColor, Color secondColor, double weight ) {
        double w  = weight * 2 - 1;
        double w1 = ( w / 1+1 ) / 2;
        double w2 = 1 - w1;
        
        long red   = Math.round( secondColor.getRed()   * w1 + firstColor.getRed()   * w2 );
        long green = Math.round( secondColor.getGreen() * w1 + firstColor.getGreen() * w2 );
        long blue  = Math.round( secondColor.getBlue()  * w1 + firstColor.getBlue()  * w2 );
        
        String hex = String. format("#%02X%02X%02X", red, green, blue);
        System.out.println( red + "," + green + "," + blue + " ||  " + hex );
        
        return new Color( (int)red, (int)green, (int)blue );
    }
    
    
}

You can invoke the class this way: Prepare some colors to sample. Must have a zero index and 100 index do create a very simple gradient. By adding colors in the middle of this range you can create nice gradients.

    public static void main(String[] args) {
        ColorGradientGenerator cg = new ColorGradientGenerator();
        
        try {
            cg.addToGradient(0,   128, 64, 0, 255);
            cg.addToGradient(50,  58, 168, 69, 255);
            cg.addToGradient(100, 255, 255, 255, 255);
            
            cg.getColorAt(0.0);
            cg.getColorAt(0.5);
            cg.getColorAt(0.8);
            cg.getColorAt(1.0);
            
        } catch (Exception e) {
            e.printStackTrace();
        }

    }

What I need to know is: Am I forgot to cover something that will lead to an error? My concern is about the things I changed/removed from the original code.

Another thing: a value of 0.8 for example will not put you at 80% of the color ramp. I can't think a global way to do this. Instead will put you at 80% of the gradient made by the color pair near (that match) 80% of the sample colors.

Lets see:

In the example I've created a ramp using 3 colors:

 cg.addToGradient(0,   128, 64, 0, 255);  // #804000
 cg.addToGradient(50,  58, 168, 69, 255); // #3aa845
 cg.addToGradient(100, 255, 255, 255, 255); // #FFFFFF

So, 0.8 will take the last pair because 80 is between 100 and 50 ( lower #3aa845 and upper #FFFFFF), create a gradient and give you the color at 80% of this ( #B0DCB5 ).

EDIT : This is the ColorItem class

package br.com.cmabreu.color;

import java.awt.Color;

public class ColorItem {
    private int index;
    private Color color;

    public ColorItem( int index, Color color ) {
        this.color = color;
        this.index = index;
    }
    
    public Color getColor() {
        return color;
    }
    
    public int getIndex() {
        return index;
    }
    
}

\$\endgroup\$
0

1 Answer 1

6
\$\begingroup\$

First, a naming nitpick. I don't think your class is a gradient generator so much as just being a gradient in and of itself. You could just call it Gradient. At that point addToGradient and removeFromGradient can just be add and remove as well

You throw Exception in multiple places. That's generally not a great thing to do, because it means that while programs using your API can tell when something goes wrong, they can't tell why, which is inconvenient. It would be much more convenient if the exceptions had more specific types. IllegalArgumentException seems like a good fit for addToGradient and some uses of getColorAt. As for getItemZero... well, IllegalStateException isn't a terrible fit, but I don't think exceptions are necessarily the best way to go here at all

The way you described it, a gradient must have colors that map to indices 0 and 100. If nobody would ever want to have a gradient without those, we can design the gradient class so that they won't need to worry about whether the gradient they have is in a usable state or not! If the constructor for a ColorGradientGenerator were to demand two Colors (one for 0 one for 100) and if we made sure they couldn't be removed later (by removeFromGradient or by other means), our library will keep all its functionality and the user won't have to worry about running into those particular errors. If the domain we're working in has rules, it's often good to have the computer keep track of those so we humans don't have to do it ourselves.

It also looks like there's no limit to how many colors you can have at the same index, which is a bit weird. And users have to deal with both the gradient index when making gradients and an index into the internal ArrayList when removing colors, which I think could get confusing. And a gradient of [(0, red), (20, blue), (100, green)] might not be the same as one of [(0, red), (100, green), (20, blue)], which I also wouldn't expect, and making sure they always behave the same feels like it might be hard (I think the current getColorAt might sometimes get that wrong). I feel like those are all signs that we're not using the right data structure for the job. We want something that stays in some kind of order even as we update it, mapping sparse index values to one value (a Color) each, or maybe two of them. That doesn't sound very much like like an ArrayList<ColorItem> to me, but it does sound a lot like a TreeMap<Integer, Color>! Or, perhaps, something more like a TreeMap<Integer, Pair<Color, Color>>. Either way by using a TreeMap we better represent the domain and we even get methods like floorKey and ceilingKey that let us find whichever keys are closest to some index of our choice, which seems like it could be very useful for implementing getColorAt

Edit

Wait, I'm a bit confused by the start of pickHex. As far as I can tell those first two expressions, for all their operators, don't do anything. Dividing by 1 shouldn't do anything, and all the other operations should cancel out. But maybe I'm missing something. If there is a reason why those first two lines look like that, I'd really suggest adding a comment to explain them, and if there isn't I'd just replace them with double w1 = weight;

\$\endgroup\$
6
  • \$\begingroup\$ WOW!! Many thanks ... it was very detailed!! \$\endgroup\$
    – Magno C
    May 31, 2021 at 2:45
  • 2
    \$\begingroup\$ "It also looks like you can have multiple colors at the same index. Which is a bit weird." The OP's code indeed has some weirdness in it, but in general allowing up to two colors at the same index in a color gradient is a desirable feature, since it allows the color to change discontinuously at that point in the gradient. \$\endgroup\$ May 31, 2021 at 9:36
  • \$\begingroup\$ Many thanks to all of you. I'll try to do my best to improve this code as you said. But for those that want to contribute to this "weirdness" here is the repo: github.com/icemagno/rampcreator \$\endgroup\$
    – Magno C
    May 31, 2021 at 21:29
  • \$\begingroup\$ @Sara J for your edit: I have no time to lapidate the original JS code in search for this kind of madness ( like the w/1+1) ... just need to solve my Java problem fast. The source of original code is in the question for comparation. Although this have potential to be a good tool I may need some help to polish it. \$\endgroup\$
    – Magno C
    May 31, 2021 at 21:33
  • 1
    \$\begingroup\$ @mdfst13 look here: jsfiddle.net/vksn3yLL line 55. Although found this very strange I decide to do not modify to avoid inject errors in to Java transcription. I'm afraid of black magic. \$\endgroup\$
    – Magno C
    Jun 1, 2021 at 21:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.