patternjavaMinor
Operations on strings in Java
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:
Explanation:
This function takes as parameter an
-
For the
-
For the
For example, if have as operation
-
First of all, I have to find where foo is:
-
Inside
-
When I find the string
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?
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:
So, in the first refactoring iteration I've ended up with:
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:
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:
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
stringmultiple times - count letters and words while looking forword(instead ofcontains)
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.