HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Operations on strings in Java

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
stringsoperationsjava

Problem

I have a program that makes operations on strings and this is the principal function of one of those operations. This works perfectly, but it is not efficient:

private void printString(ArrayList operations, ArrayList set) {
    int numerOfStrings = 0;
    int numberOfletters = 0;
    String toPrint = operations.get(1);
    outOfLoop: for (int i = 0; i < set.size(); i++) {
        String[] toFind = set.get(i).split(" ");
        for (int k = 0; k < toFind.length; k++) {
        if (toPrint.equals(toFind[k])) {
            String[] splited = set.get(i).split(" ");
            for (int j = 0; j < splited.length; j++) {
            numberOfletters += splited[j].length();
            }
            numerOfStrings = splited.length;
            break outOfLoop;
        }
      }  
    }
    System.out.println(numerOfStrings + " " + numberOfletters);
}


Explanation:

This function takes as parameter an arrayList of operations, and an arrayList of set:

-
For the arrayList of operations, I get always a specific position, so I don't iterate. It is always \$O(1)\$.

-
For the arrayList of set, I have to iterate, or rather, as I think of that to proceed:

For example, if have as operation print foo, I have to do these steps:

-
First of all, I have to find where foo is:

-
Inside set, I can have this situation:

position 1 : {car tree hotel}
...
position n : {foo lemon coffee}


-
When I find the string foo, I have to print the number of strings inside that position and the number of letters of each string, so in this case, I will print:

3(number of strings) 14(sum of number of letters)

My program works as well as this function, but it is a nasty and inefficient solution. How can I improve the efficiency of my program?

Solution

Let me first do a review without getting into the logic of the method.
Before I start making changes I've commented some smelly places in your original code:

//Try to use the most generic type in your arguments. You don't really need an ArrayList<>
//a List<> will suffice.
private void printString(ArrayList operations, ArrayList set) {     
    int numerOfStrings = 0;
    int numberOfletters = 0;
    //why take a list parameter when you only use one value (the second element?!)
    String toPrint = operations.get(1); 
    outOfLoop: for (int i = 0; i < set.size(); i++) {
        String[] toFind = set.get(i).split(" ");
        for (int k = 0; k < toFind.length; k++) {
            if (toPrint.equals(toFind[k])) {
                // duplicated work: toFind == splitted
                String[] splited = set.get(i).split(" "); 
                for (int j = 0; j < splited.length; j++) {
                    numberOfletters += splited[j].length();
                }
                numerOfStrings = splited.length;
                //goto is considered evil
                //Besides, goto to label is not neccessary here.
                //A simple `break` would suffice
                break outOfLoop;  
            }
        }
    }
    //you should return the results and let the caller of the method
    //decide what to do with them
    System.out.println(numerOfStrings + " " + numberOfletters);
}


So, in the first refactoring iteration I've ended up with:

public static class Result {
    public final int numerOfStrings;
    public final int numberOfletters;

    public Result(int numerOfStrings, int numberOfletters) {
        this.numerOfStrings = numerOfStrings;
        this.numberOfletters = numberOfletters;
    }

    @Override
    public String toString() {
        return numerOfStrings + " " + numberOfletters;
    }
}

private Result process(String word, List set) {
    int numerOfStrings = 0;
    int numberOfletters = 0;

    for (int i = 0; i < set.size(); i++) {
        String[] toFind = set.get(i).split(" ");
        for (int k = 0; k < toFind.length; k++) {
            if (word.equals(toFind[k])) {
                String[] splited = set.get(i).split(" ");
                for (int j = 0; j < splited.length; j++) {
                    numberOfletters += splited[j].length();
                }
                numerOfStrings = splited.length;
                break;
            }
        }
    }
    return new Result(numerOfStrings, numberOfletters);
}

@Test
public void run() throws Exception {
    Result result = process("foo", Arrays.asList("car tree hotel","foo lemon coffe"));
    System.out.println(result);
}


This keeps the method logic intact, but fixes all the issues I mentioned in the comments.

But actually I think the method could be a lot simpler:

private Result process(String word, Iterable set) {
    for (String string : set) {
        if(string.contains(word)) {
            //" +" is a regex patten for "one or more spaces"  
            int numberOfWords = string.trim().split(" +").length;
            int numberOfLetters = string.replace(" ", "").length();
            return new Result(numberOfWords,  numberOfLetters );
        }
    }
    return new Result(0, 0);
}


This version is more readable, but could be optimized for performance. As always with performance you should first check if optimizing here would make any impact on the application as a whole. Sacrificing readability might not be worth it.

Optimizations might include:

  • avoid new string creation using trim()



  • avoid new string array creation using split()



  • avoid new string creation using replace()



  • avoid iterating over string multiple times - count letters and words while looking for word (instead of contains)

Code Snippets

//Try to use the most generic type in your arguments. You don't really need an ArrayList<>
//a List<> will suffice.
private void printString(ArrayList<String> operations, ArrayList<String> set) {     
    int numerOfStrings = 0;
    int numberOfletters = 0;
    //why take a list parameter when you only use one value (the second element?!)
    String toPrint = operations.get(1); 
    outOfLoop: for (int i = 0; i < set.size(); i++) {
        String[] toFind = set.get(i).split(" ");
        for (int k = 0; k < toFind.length; k++) {
            if (toPrint.equals(toFind[k])) {
                // duplicated work: toFind == splitted
                String[] splited = set.get(i).split(" "); 
                for (int j = 0; j < splited.length; j++) {
                    numberOfletters += splited[j].length();
                }
                numerOfStrings = splited.length;
                //goto is considered evil
                //Besides, goto to label is not neccessary here.
                //A simple `break` would suffice
                break outOfLoop;  
            }
        }
    }
    //you should return the results and let the caller of the method
    //decide what to do with them
    System.out.println(numerOfStrings + " " + numberOfletters);
}
public static class Result {
    public final int numerOfStrings;
    public final int numberOfletters;

    public Result(int numerOfStrings, int numberOfletters) {
        this.numerOfStrings = numerOfStrings;
        this.numberOfletters = numberOfletters;
    }

    @Override
    public String toString() {
        return numerOfStrings + " " + numberOfletters;
    }
}

private Result process(String word, List<String> set) {
    int numerOfStrings = 0;
    int numberOfletters = 0;

    for (int i = 0; i < set.size(); i++) {
        String[] toFind = set.get(i).split(" ");
        for (int k = 0; k < toFind.length; k++) {
            if (word.equals(toFind[k])) {
                String[] splited = set.get(i).split(" ");
                for (int j = 0; j < splited.length; j++) {
                    numberOfletters += splited[j].length();
                }
                numerOfStrings = splited.length;
                break;
            }
        }
    }
    return new Result(numerOfStrings, numberOfletters);
}


@Test
public void run() throws Exception {
    Result result = process("foo", Arrays.asList("car tree hotel","foo lemon coffe"));
    System.out.println(result);
}
private Result process(String word, Iterable<String> set) {
    for (String string : set) {
        if(string.contains(word)) {
            //" +" is a regex patten for "one or more spaces"  
            int numberOfWords = string.trim().split(" +").length;
            int numberOfLetters = string.replace(" ", "").length();
            return new Result(numberOfWords,  numberOfLetters );
        }
    }
    return new Result(0, 0);
}

Context

StackExchange Code Review Q#52603, answer score: 4

Revisions (0)

No revisions yet.