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.

For practicing reasons I would like to write a method which inserts a character into a string.

I would like to know:

  1. What is the best practice concerning placement of a comment within methods? For example, is the way that I commented about what raises an exception fine, or are there better practices? Would I need more comments, and if so, where?

  2. In this particular example, are throwing unchecked exceptions preferred or checked?

  3. How can I make this method more reliable?

  4. How can I make this method more scalable?

  5. While dealing with immutable strings there are always discussions around performance. Can I make this method run faster?

  6. Is this code thread safe?

/**
 * Inserts the character ch at the location index of string st
 * @param st
 * @param ch
 * @param index
 * @return a new string 
 */
    public static String insertCharAt(String st, char ch, int index){
        //1 Exception if st == null
        //2 Exception if index<0 || index>st.length()

        if (st == null){
            throw new NullPointerException("Null string!");
        }

        if (index < 0 || index > st.length())
        {
            throw new IndexOutOfBoundsException("Try to insert at negative location or outside of string");
        }
        return st.substring(0, index)+ch+st.substring(index, st.length());
    }
share|improve this question
add comment

2 Answers 2

  1. The Javadoc style headers you have provided for this method are already more than enough. If you are adding comments to your method it should only be either for API documentation, or where the code is sufficiently complex to warrant an explanation that you cannot provide by simplifying or more appropriately naming the variables.

    You should however make it clear that your method throws these two exceptions by including them in the method definition, e.g.

    public static String insertCharAt(String str, char ch, int index) throws NullPointerException, IndexOutOfBoundsException
    

    This way it will be obvious from reading the method definition that these exceptions can be thrown, and your comment will be unnecessary when we read the line above each exception-throwing statement and see the logic that causes it to be thrown.

    In general, if your comment is telling us nothing more than what the code already does, make your code as simple and readable as possible and delete the comment. At best it will add noise to the code file and at worst it will get out of sync with the code and become misleading.

  2. In this example, presumably prefer to use a checked exception so that the method calling insertCharAt and can catch these exceptions and handle them in some way. See also http://stackoverflow.com/questions/27578/when-to-choose-checked-and-unchecked-exceptions

  3. This depends what you mean by more reliable. Do you mean robust in regards error cases?

  4. This also depends on what you mean by more scalable. What do you mean?

  5. This method is very simple and there isn't a great deal of room for performance improvement that I can see.

    Edit:

  6. This method is thread safe as it does not modify any parameters passed to it (and strings are immutable anyway) and does not modify any class-local variables. Even if you had assigned the return value to a temporary variable before returning it, it would not be shared between threads as method level variables belong to each thread's stack.

share|improve this answer
add comment
  1. Comments - in methods are only useful if they tell you something that is not immediately obvious when you read the code. Your code is immediately obvious, and thus the comments inside the method are completely redundant. I have a note a little later about your JavaDoc though....

  2. Exceptions - In many ways the difference between when checked and unchecked exceptions are useful, is easy to determine:

    • If something fails because the programmer using your code is an idiot (or needs a smack to the side of the head), then throw an unchecked exception.
    • If something fails because of an environmental situation beyond the control of the programmer, then throw a checked exception.

    In your case, if the programmer gives you garbage, throw an unchecked exception.... and there is essentially nothing environmental that can go wrong. So, no checked exceptions from your method.

  3. Reliability - what can go wrong? In this case, right now, nothing. If something goes wrong it is either because the person using the method is an idiot, or you are an idiot. in either case, you will immediately be aware of that. Your code is simple, and works. For larger functions, it may not be so clear...

    But, reliability is about more than what is happening now. Reliability is making sure that things work in the future too. So, to improve reliability, write tests. Make the tests run every conceivable condition through the code. Run the tests every time you change the code.

    That is what makes code reliable. Tests. Tests, tests, and more tests. Then Regression tests, and compatibility tests. There is a common theme here, can you spot it?

  4. Scalability - this is a buzz-word that that you threw in because right now it is popular. What influences could introduce scalability problems in your code. In essence, only one thing: very large Strings. With current and older versions of Java (less than 3 or 4 months old), the memory is shared between String values. In your case, doing String.substring(....) does not take much more memory (about 24 to 64 bytes depending on the JVM). In newer versions of Java (recent Java7 fic-packs and Java8), the substring command will copy the relevant inner memory of the String, and thus will take much more memory. Your code could have scalability problems since it will require a large amount of additional memory each time you do an insert.

    Now, whether that is a thing you should worry about.... no, I don't think so. If you have Strings that large, you will have other problems first.

  5. Performance - Yes, it can be faster. Is the improvement meaningful, I doubt it, but, for the record:

    • you are checking your input values for nulls, and index constraint violations. These exceptions will be thrown anyway, but with default messages. If you remove the explicit checks, you will not actually lose any functionality (except for less meaningful exception messages), and you will gain performance.

    • The String concatenation in this line here:

      st.substring(0, index)+ch+st.substring(index, st.length())
      

      is creating a new StringBuilder instance, two new String objects, concatenating them, and then converting the result to a String. By my count that is three Strings, and a StringBuilder.

      You can do better by having just two char[] arrays, and a String.

      char[] chars = str.toCharArray();
      chars = Arrays.copyOf(chars, chars.length + 1);
      System.arrayCopy(chars, index, chars, index + 1, chars.length - index - 1);
      chars[index] = ch;
      return new String(chars);
      

      Whether the marginal improvement will help you or not is uncertain....

  6. Thread-Safety - no problems.... there are no external references, and it is fully 'A-OK'.

other:

I am a firm believer in JavaDoc. Depending on your circumstances, JavaDoc can be less important. Any time I wrote code that is more 'serious' (I expect anyone to have to maintain), I write comprehensive JavaDoc. IDE's make this relatively easy. Your JavaDoc is not complete.

You do not describe the input values appropriately, and there is not enough detail in the return section.

As for declaring the thrown unchecked exceptions.... no. Don't do it... see this Exception Tutorial here, but, you should document them anyway in the JavaDoc.

The JavaDoc is your communication with other programmers. Help them by giving good documentation.

Similarly, your actual exceptions are your communication with other programmers... help them too. Currently your exceptions are poor... consider these changes:

throw new NullPointerException("Null String input value: st");

and

throw new IndexOutOfBoundsException("No such index " + index
                   + " in a String of size " + st.length()
                   + ". Expect index value 0 <= index <= " + st.length());
share|improve this answer
    
The two char array approach might have some insignificant performance increase but think of the poor future programmers who have to come along and understand that code! The readability and maintainability tradeoffs are surely worth keeping it simple. –  willh Mar 14 at 21:30
    
@willh - yeah, that is why I reluctantly suggested it. There are times when the performance difference makes a difference. I have no idea of whether this is the time, or not. –  rolfl Mar 14 at 21:35
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.