3
\$\begingroup\$

I really need some help removing some duplicated code. I've linked the methods causing me issues. http://pastebin.com/cZJihM4J

As you can see each method does similar things but retrieve different values. i.e one method to insert a string at a certain line. Another to get the the lines index and another to get the contents at a particular line

There are things like:

String text = textArea.getText();
int start = 0;
int count = 0;
String buildNewTextArea = "";

I could make these values global but not sure if this is the best way to approach this. My brain can't find away around this :(

Thanks in advance

 public String insertCodeBlock(String newBlock) {
    String text = textArea.getText();
    int start = 0;
    int count = 0;
    String buildNewTextArea = "";
    while (start >= 0) {
        int nextLineStart = text.indexOf(NEW_LINE, start + NEW_LINE.length());
        if (nextLineStart == -1) {
            nextLineStart = text.length();
        }
        String[] temp = diffBlocks.get(indexOfLine).split(",");
        int startLine = Integer.valueOf(temp[0]);
        int endLine = (Integer.valueOf(temp[1]) - 1) + startLine;
        if (count == startLine) {
            buildNewTextArea += newBlock;
        } else if (count < startLine || count > (startLine + (endLine - startLine))) {
            buildNewTextArea += text.substring(start, nextLineStart);
        }
        count++;
        start = text.indexOf(NEW_LINE, nextLineStart);
    }
    return buildNewTextArea;
}

private int getLineIndex(int line) {
    String text = textArea.getText();
    int start = 0;
    int count = 0;
    String buildNewTextArea = "";
    int index = 0;
    while (start >= 0) {
        int nextLineStart = text.indexOf(NEW_LINE, start + NEW_LINE.length());
        if (nextLineStart == -1) {
            nextLineStart = text.length();
        }
        if (count == line) {
            return index;
        }
        buildNewTextArea = text.substring(start, nextLineStart);
        index += buildNewTextArea.length();
        count++;
        start = text.indexOf(NEW_LINE, nextLineStart);
    }
    return text.length();
}


public String getLineContent(int startLine, int endLine) {
    String text = textArea.getText();
    int start = 0;
    int count = 0;
    String buildNewTextArea = "";
    while (start >= 0) {
        int nextLineStart = text.indexOf(NEW_LINE, start + NEW_LINE.length());
        if (nextLineStart == -1) {
            nextLineStart = text.length();
        }
        if (count >= startLine && count <= endLine) {
            buildNewTextArea += text.substring(start, nextLineStart);
        }
        count++;
        start = text.indexOf(NEW_LINE, nextLineStart);
    }
    return buildNewTextArea;
}
\$\endgroup\$

migrated from stackoverflow.com Dec 13 '11 at 17:43

This question came from our site for professional and enthusiast programmers.

  • 2
    \$\begingroup\$ Globals are never the best way to approach anything. \$\endgroup\$ – Brian Driscoll Dec 13 '11 at 16:53
  • 1
    \$\begingroup\$ Don't do this with Strings: buildNewTextArea += newBlock in a loop. You should use a StringBuilder instead: it's more verbose but also much more efficient (you'll be generating way less garbage). \$\endgroup\$ – user988052 Dec 13 '11 at 17:23
1
\$\begingroup\$

Maybe this can help?

private static int getLineLength(String str, int line){
        return str.split("\n")[line].length(); //line start 0
    }
    private static String getLineContent(String str, int startLine, int endLine){
        String s = "";for(int i=startLine;i<=endLine;i++){s +=str.split("\n")[i] ;}return s;
    }
    private static String insertBlock(String sourceStr, String insertStr, int startLine, int endLine){
    return getLineContent(sourceStr, 0, startLine).concat(insertStr).concat(getLineContent(sourceStr, endLine, sourceStr.split("\n").length-1));    
    }
\$\endgroup\$
2
\$\begingroup\$

It seems like each method is iterating over the list of lines and doing something. It is the "doing something" that is unique. Consider having a single method that iterates over the lines and takes an interface to do the "something". For each method, an appropriate interface instance would be passed (possible anonymous inner class) that takes the appropriate action.

Something like this:

private interface ProcessLine{
     void processLine(String line, int index, int charIndex);
     void complete();
}

private ProcessLine process(ProcessLine processor){
  String text = textArea.getText();
  int start = 0;
  int count = 0;
  String buildNewTextArea = "";
  int index = 0;
  while (start >= 0) {
    int nextLineStart = text.indexOf(NEW_LINE, start + NEW_LINE.length());
    if (nextLineStart == -1) {
        nextLineStart = text.length();
    }

    processor.processLine(line, count);
    index += buildNewTextArea.length();
    count++;
    start = text.indexOf(NEW_LINE, nextLineStart);
  }
  return processor;
}

private int getLineIndex(final int line) {
     int result = -1;
     return process(new ProcessLine(){
         public void processLine(String line, int index, int charIndex){
             if (index == line)
                 result = charIndex;
         }
     });
     return result;
}
\$\endgroup\$
1
\$\begingroup\$

In the spirit of 'clean code', you should strive to make your methods as short as possible, and they should do only one thing.

To start with, the following is a candidate for extracting into its own method

   int nextLineStart = text.indexOf(NEW_LINE, start + NEW_LINE.length());
    if (nextLineStart == -1) {
        nextLineStart = text.length();
    }

I would also be tempted to make the following class instance variables

String text = textArea.getText();
int start = 0;
int count = 0;

That should reduce a lot of the duplicated code you have, and the rest looks to be specific to each method.

\$\endgroup\$
  • 1
    \$\begingroup\$ Disagree on the instance variable, you could run into thread-safety issues. Variables should be at the lowest possible scope. \$\endgroup\$ – John B Dec 13 '11 at 17:10
  • \$\begingroup\$ Agree on the clean code. Anything that is duplicate could be moved to a method. \$\endgroup\$ – John B Dec 13 '11 at 17:11
  • \$\begingroup\$ Agreed, but the OP didn't mention anything about multi-threading. I don't think there's anything bad about using private class instance variables, after all they are limited in scope to the enclosing class. \$\endgroup\$ – Andrew Fielden Dec 13 '11 at 17:12
  • 1
    \$\begingroup\$ I will still stay with the rule that if a variable is never used outside a method (or its value does not need to be retained between method calls) it should be method-scope (the lowest possible scope). IMHO \$\endgroup\$ – John B Dec 13 '11 at 17:15
  • \$\begingroup\$ Fair comment. If the variable is very localised and only used in one place, then would be better as a method local. \$\endgroup\$ – Andrew Fielden Dec 13 '11 at 17:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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