current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I'm making my own script editor (in this case for Arma: Cold War Assault) because I want to learn and this is challenging enough.

Let me just get this out of the way: please don't tell me that I should do easier things. I want to do this anyway.

So, basically I have the simple GUI for now with a working new/open/save file menu.

I've managed to highlight certain words with different colors (because I want to tackle the hardest part first) but it's not efficient.

I've come up with several ideas for the algorithm (didn't implement them all), but I want to know what you'do, if there's a certain way and what I am doing wrong.

This all happens inside a JTextPane class.

The arrays containing the reserved words:

Collections.addAll(keywords, "private", "public", "if", "not", "then", "else", "else if");          
Collections.addAll(operators, "+", "-", "=", "==", "?", "!","(", ")","{", "}", "_", "-", "^", "<", ">");            

ArrayList<String> keywords = new ArrayList<String>();
ArrayList<String> operators = new ArrayList<String>();

Everytime the user makes an update to the document, it get's updated:

@Override
public void insertUpdate(DocumentEvent e) {
    update();
}

@Override
public void removeUpdate(DocumentEvent e) {
    update();
}

When the user stops typing, it waits 500 ms to update the screen: 

Timer t;

/**
 * Updates the text when user stops typing
 */
public void update(){

    if (t != null) {
        if (t.isRunning())
            t.stop();
    }

    t = new Timer(250, new ActionListener() {

        @Override
        public void actionPerformed(ActionEvent e) {

            long start = System.currentTimeMillis();

            String text = getText();                

            SimpleAttributeSet attrs = new SimpleAttributeSet();
            StyleConstants.setForeground(attrs, Color.BLACK);
            StyledDocument doc = getStyledDocument();
            doc.setCharacterAttributes(0, text.length(), attrs, true);

            int index = 0, carriage = 0;

            while ((index < text.length())) {

                if (text.codePointAt(index) == 10) {
                    carriage += 1;
                }

                for (String s : Keywords.listKeywords()) {                      
                    if (text.startsWith(s, index)){
                        changeColor(doc, reserved.get(s), index, carriage, s.length(), false);
                        index += s.length();
                        break;
                    }
                }

                for (String s : Keywords.listOperators()) {
                    if (text.startsWith(s, index)){
                        changeColor(doc, reserved.get(s), index, carriage, s.length(), false);
                        index += s.length();
                        break;
                    }
                }

                index++;
            }
            wordCount.setText("word count: " + index);
            System.out.println("Iterations took: "
                    + (System.currentTimeMillis() - start) + " ms");

            t.stop();
        }
    });
    t.start();
}

private void changeColor(StyledDocument doc, Color color, int index, int carriage, int length, boolean replace) {
    SimpleAttributeSet attrs = new SimpleAttributeSet();
    StyleConstants.setForeground(attrs, color);
    doc.setCharacterAttributes(index - carriage, length, attrs, replace);
}

How would I go about doing this more efficiently?

As of right now, with almost 145.000 words i have a delay of about 400 ms with 12 keywords and other words.

share|improve this question
    
scanner.match().start() returns 0 whenever it starts a new line, so it doesn't know it is in a new line... –  Jh62 Jul 26 '13 at 5:55
    
The code in revision 4 does not work so I've rollbacked it since it's off-topic here. I think it should be on Stack Overflow instead. –  palacsint Jul 26 '13 at 6:42
add comment

2 Answers

up vote 1 down vote

As a general direction for improvement, your technique for finding those keywords uses startsWith, which will effectively compare any given character once per potential keyword or operator. If you switch to regular expressions, by compiling a Pattern that matches any keyword or any operator, you build (what amounts to) a lookup table that typically allows one comparison per character, reducing the total number of comparisons significantly.

You can even use Pattern.quote to make building the String easier.

public Pattern fromList(List<String> strings) {
  StringBuilder builder = new StringBuilder();
  boolean isFirst = true;
  for (String string : strings) {
    if (!isFirst) {
      builder.append('|');
    }
    builder.append(Pattern.quote(string));
    isFirst = false;
  }
  return Pattern.compile(builder.toString());
}

After that, search for every occurrence of the String using either Pattern.matcher() (or Scanner.next()), which will save you those repeated comparisons. At that point you can use the start and end methods on Matcher (or the same on Scanner.match()).

I do notice that your algorithm will tend to highlight the first half of terms like publicValue even if the compiler does not detect it as a keyword. For this, you may want to customize the patterns you generate to match word boundaries.

share|improve this answer
    
Thanks. Patterns are complicated for first use! I understand how it works, but: how would I make a pattern to match a set of fixed words, like "public, private, int, boolean". I managed to understand how to use to search for a pattern, but not for individual words. –  Jh62 Jul 25 '13 at 4:48
    
Im using right now: Matcher m = Pattern.compile("\\b(public+?)\\b").matcher(this.getText()); but surelly there are better ways i imagine. –  Jh62 Jul 25 '13 at 5:00
    
The pipe symbol (|) means "or". Try "\\b(public|private|function)\\b" and so forth. Because you're in parens, the word would technically be "group 1" for the methods on Matcher, but since word boundaries are zero-length it'd work the same. Don't be afraid to generate the regex from your existing List. ;) –  Jeff Bowman Jul 25 '13 at 6:20
add comment
up vote 0 down vote

Currently the it's O(n) where n is the length of the text inside the JTextPane. You could reduce it if you process only the modified regions of the document in the event listener. I think DocumentEvent's getLength() and getOffset() method could help here.

Another notes:

  1. int index = 0, carriage = 0;

    I'd put the variable declarations to separate lines. From Code Complete, 2nd Edition, p759:

    With statements on their own lines, the code reads from top to bottom, instead of top to bottom and left to right. When you’re looking for a specific line of code, your eye should be able to follow the left margin of the code. It shouldn’t have to dip into each and every line just because a single line might contain two statements.

  2. Be aware of static helper classes, like Keywords. You might find my former answer about it useful.

share|improve this answer
1  
Thanks! Didn't search through all the DocumentEvent's methods. getOffset() is perfect. –  Jh62 Jul 25 '13 at 5:02
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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