4
\$\begingroup\$

I am checking for particular keywords in a string, and then changing the foreground and background colors of substrings based on the keywords. Following is the snippet for checking the background code:

if(string.contains("[30;42m") || string.contains("[31;42m") || string.contains("[32;42m") ||
    string.contains("[33;42m") || string.contains("[34;42m") || string.contains("[35;42m")
    || string.contains("[36;42m") || string.contains("[37;42m")){
        //the parameter of indexOf and replaceAll will be one from the above
        //using [30;42m as an example
        index = string.indexOf("[30;42m"); 
        string = string.replaceAll("\\[30;42m", "");
        backgroundColor = Color.GREEN;
        //foreground is already set using separate conditions for 30,32,32...37m
     }

Is there any way that I can do something as I have shown above? Currently, I have separate conditions for each substring, which doesn't incorporates "DRY" principle. My code is working, but I am looking for a way to reduce these if statements.

Edit: The if blocks are for setting the background and foreground colors. For example if I have "34;42" code, this means text foreground color is Blue and background color is Green, and so on.

\$\endgroup\$
1
  • 3
    \$\begingroup\$ Could you please provide a little more conetext on the implementation inside the if-block? I strongly suspect the refactoring strategy will change, depending on how you impelemented the body. \$\endgroup\$
    – Vogel612
    Commented May 9, 2014 at 7:31

3 Answers 3

5
\$\begingroup\$

You can do it a little more generic but I'm afraid you still have to test each.

I don't know if your case is always changing to green or not.
If not I suggest the following enum :

/**
 *
 * @author chillworld
 */
public enum ColorMapperEnum {
    GREEN("[30;42m",Color.GREEN) ,
    RED("[31;42m",Color.RED) ,
    BLACK("[32;42m", Color.BLACK) ,
    BLUE("[33;42m", Color.BLUE); // and so on

    private String textToFind;
    private Color targetColor;

    private ColorMapperEnum(String textToFind, Color targetColor) {
        this.textToFind = textToFind;
        this.targetColor = targetColor;
    }

    public String getTextToFind() {
        return textToFind;
    }

    public Color getTargetColor() {
        return targetColor;
    }
}

Now you can do a whole lot more generic in your java code :

for (ColorMapperEnum colorMapper : ColorMapperEnum.values()) {
    if(string.contains(colorMapper.getTextToFind()){
        index = string.indexOf(colorMapper.getTextToFind()); 
        string = string.replaceAll("\\" + colorMapper.getTextToFind(), "");
        backgroundColor = colorMapper.getTextToFind();
    }
 }

I did use the same naming as yo but I advice to take better naming.
String string => very bad, for what stands it?

Now I don't know if there could be a case that multiple conditions are true and what there must be done when multiple conditions are true.

An enum values() has always the same result in the same order. Use that in your advantage when tackling that problem.

\$\endgroup\$
7
  • \$\begingroup\$ Thank you for your feedback, yes you are right. I just need to set three values instead of two: textToFind, foregroundColor, BackgroundColor \$\endgroup\$
    – w_ahmed
    Commented May 9, 2014 at 9:52
  • \$\begingroup\$ One more thing, willn't it be better from performance viewpoint if I use a hash table? \$\endgroup\$
    – w_ahmed
    Commented May 9, 2014 at 10:09
  • \$\begingroup\$ where exactly? in stead of the enum? \$\endgroup\$
    – chillworld
    Commented May 9, 2014 at 12:04
  • \$\begingroup\$ Yes,instead of using enum. \$\endgroup\$
    – w_ahmed
    Commented May 9, 2014 at 12:19
  • 2
    \$\begingroup\$ Actually Enum is better. You have to construct once. Always singleton and accesible in each class. If you need foreground also change targetColor to backgroundColor and add a color for foregroundColor. Add the second color in the constructor and values of the enum. (if help needed ask and I refactor code here). Is it possible that 30 and 31 are in the same string? \$\endgroup\$
    – chillworld
    Commented May 9, 2014 at 12:39
0
\$\begingroup\$

One idea I'd have here is maybe use a different approach and parser your string into Tokens.

Have a Token each for the color markers (a simple 2-tuple/pair implementation will do) and the non-marker strings in between.

So a string such as

This[30;42m is an[31;42m Example

would become a list like (pseudocode)

String("This"), Tuple(30, 42), String(" is an"), Tuple(31, 42), String(" Example")

Now you just need to iterate over the list, compare each tuple with the set of tuples you are looking for, building a new list that you can finally turn back into a string.

I hope this is understandable.

\$\endgroup\$
0
\$\begingroup\$

It appears that you want to support 8 background colors, and probably 8 foreground colors as well. That would be 64 magic strings that you are looking for, with 64 cases to handle them all. With so many cases, string matching is inappropriate; you want to do pattern matching. Pattern matching should be done using regular expressions.

Furthermore, you're probably better off writing a proper ANSI terminal emulator, or at least just a state machine to support for changing colors. The following class breaks up an input string into multiple colored strings.

import java.awt.Color;
import java.util.Iterator;
import java.util.NoSuchElementException;
import java.util.regex.*;

public class ANSIAttrString implements Iterable<ANSIAttrString> {

    //////////////////////////////////////////////////////////////////////

    public static class TermState {
        public static final TermState DEFAULT = new TermState(Color.WHITE, Color.BLACK);

        public final Color fg, bg;

        public TermState(Color fg, Color bg) {
            this.fg = fg;
            this.bg = bg;
        }

        public TermState(TermState init, String[] attrs) {
            Color fg = init.fg;
            Color bg = init.bg;
            for (String attr : attrs) {
                switch (attr) {
                  case  "0": fg = DEFAULT.fg;
                             bg = DEFAULT.bg;
                             break;

                  case "30": fg = Color.BLACK;          break;
                  case "31": fg = Color.RED;            break;
                  case "32": fg = Color.GREEN;          break;
                  case "33": fg = Color.YELLOW;         break;
                  case "34": fg = Color.BLUE;           break;
                  case "35": fg = Color.MAGENTA;        break;
                  case "36": fg = Color.CYAN;           break;
                  case "37": fg = Color.WHITE;          break;
                  case "39": fg = DEFAULT.fg;           break;

                  case "40": bg = Color.BLACK;          break;
                  case "41": bg = Color.RED;            break;
                  case "42": bg = Color.GREEN;          break;
                  case "43": bg = Color.YELLOW;         break;
                  case "44": bg = Color.BLUE;           break;
                  case "45": bg = Color.MAGENTA;        break;
                  case "46": bg = Color.CYAN;           break;
                  case "47": bg = Color.WHITE;          break;
                  case "49": bg = DEFAULT.bg;           break;
                }
            }
            this.fg = fg;
            this.bg = bg;
        }

        public String toString() {
            return fg + " on " + bg;
        }
    }

    //////////////////////////////////////////////////////////////////////

    private static class Iter implements Iterator<ANSIAttrString> {
        private final String s;
        private Matcher m;
        private TermState state;
        private int index;

        Iter(String s, TermState init, Matcher m) {
            this.s = s;
            this.state = init;
            this.m = m;
        }

        public boolean hasNext() {
            return this.m != null;
        }

        public ANSIAttrString next() {
            if (this.m == null) {
                throw new NoSuchElementException();
            }

            int thisEnd = m.find() ? m.start() : m.regionEnd();
            try {
                return new ANSIAttrString(s.substring(index, thisEnd), this.state);
            } finally {
                if (m.hitEnd()) {
                    this.m = null;
                } else {
                    String[] attrs = m.group(1).split(";");
                    state = new TermState(state, attrs);
                    this.index = m.end();
                }
            }
        }

        public void remove() {
            throw new UnsupportedOperationException();
        }
    }

    //////////////////////////////////////////////////////////////////////

    public final TermState state;
    public final String string;

    private static final Pattern SGR_PATTERN = Pattern.compile(
        "\\e\\[(\\d+(?:;\\d+)*)m"
    );

    public ANSIAttrString(String string) {
        this(string, TermState.DEFAULT);
    }

    public ANSIAttrString(String string, TermState state) {
        this.state = state;
        this.string = string;
    }

    public Iterator<ANSIAttrString> iterator() {
        return new Iter(this.string, this.state, SGR_PATTERN.matcher(this.string));
    }

}

Test case:

public static void main(String[] args) {
    String str = "It's a \u001b[31mRainbow\u001b[34;43m colored\u001b[39m world.";
    for (ANSIAttrString ats : new ANSIAttrString(str)) {
        System.out.println(ats.state + ": " + ats.string);
    }
}
java.awt.Color[r=255,g=255,b=255] on java.awt.Color[r=0,g=0,b=0]: It's a 
java.awt.Color[r=255,g=0,b=0] on java.awt.Color[r=0,g=0,b=0]: Rainbow
java.awt.Color[r=0,g=0,b=255] on java.awt.Color[r=255,g=255,b=0]:  colored
java.awt.Color[r=255,g=255,b=255] on java.awt.Color[r=255,g=255,b=0]:  world.
\$\endgroup\$

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.