Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I'm a scala beginner and looking at the trim() method of the java api I noticed side effects, so I attempted to implement a functional version in scala.

Here is the java version : 

public String trim() {
int len = count;
int st = 0;
int off = offset;      /* avoid getfield opcode */
char[] val = value;    /* avoid getfield opcode */

while ((st < len) && (val[off + st] <= ' ')) {
    st++;
}
while ((st < len) && (val[off + len - 1] <= ' ')) {
    len--;
}
return ((st > 0) || (len < count)) ? substring(st, len) : this;
}

My scala version iterates over a string & when it encounters a space char the tail of the string is returned. The entire string is then reversed, so can access the trailing whitespace. This new string is iterated over until a space char is encounterd and the tail of this string is returned. This new string is then reversed again to maintain the original string order : 

def trim(stringToTrim : String) : String = {
    def removePreString[String](list : List[String]) : java.lang.String = list match {

        case head :: tail => {
            if(head.toString.equals(" ")){
                removePreString(tail)
            }
            else {
                head.toString + tail.mkString
            }
        }
    }//end removePreString

    val preString = removePreString(stringToTrim.toList)
    val reveresedString = preString.toList.reverse
    val reveresedPostString = removePreString(reveresedString)
  reveresedPostString.reverse

}//end trim

How can this code be improved ? It seems wasteful reversing the string twice ?

share|improve this question

3 Answers

up vote 2 down vote accepted

No good idea about the concern of reversing the list twice. But you may

1) let you inner function return a List instead of a string thus having less conversions from list to string and string to list.

2) change your match to the following pattern (not tested):

case " " :: tail => removePreString(tail)
case _ => _

I just realize you could also use on a list def dropWhile(p: (A) ⇒ Boolean): List[A] which drops longest prefix of elements that satisfy a predicate. You could then chain dropWhile not a space | reverse | dropwhile not a space | reverse...

share|improve this answer
up vote 1 down vote

you can test for a whitespace in patter matching, an alternative version of your removePreSign method therefore could be written as:

def removePreString(list : List[Char]): String = list match {
  case ' ' :: tail => removePreString(tail)
  case _ => list.mkString
}

but as pgras noted, you can simply use str.dropWhile(_ == ' ') which avoids the whole String <=> List issue.

A possible way to do it without reverse and dropWhile using only recursion:

def trim(stringToTrim : String) : String = {
  def removeLeading(str: String): String =
    if (str startsWith " " ) removeLeading(str tail)
    else str

  def removeTrailing(str: String): String =
    if (str endsWith " ") removeTrailing(str dropRight 1 )
    else str

  removeTrailing(removeLeading(stringToTrim))
}
share|improve this answer
your function 'removePreString' does not work for Strings with trailing whitespace. e.g - " this is a test " – user470184 Jan 14 at 21:38
neither does the original function, it is just another way to achieve the same thing, but as noted above it is equivalent to dropWhile – Predator117 Jan 15 at 9:31
up vote 1 down vote
  • Since left and right trim may be useful on their own, expose them.
  • View String as a list of Char instead of converting to List[String]
  • The recursion creates a string at each step ! Take advantage of the rich library of methods for lists (and the like).
  • For instance, reversing twice is fine as long you manipulate iterators.

Applying these advices leads to :

def trimLeft  (s: String) = s dropWhile ( _ == ' ' )
def trimRight (s: String) = (s.reverseIterator dropWhile ( _ == ' ' ))
                               .toSeq.reverseIterator mkString
val trim = trimLeft _ compose trimRight _

It you prefer to extract the sub-string of interest at once, there are fine functions to find the indices you need.

share|improve this answer

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.