Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

Two code files below : the actual logic & the test logic.

Core Logic

package stringBuilder;

public class NoksStringBuilder {

    //initial capacity
    private static final int INITIAL_SIZE = 3;

    String[] stringList = new String[INITIAL_SIZE];

    //number of strings in the builder currently
    int size = 0;

    //total number of chars in the builder , sum total of length of each string
    int characterCount = 0;


    public void add(String s){
        if(size < stringList.length){
            stringList[size++] = s;
            characterCount += s.length();
        }
        else{
            String[] temp = new String[stringList.length*2];

            for(int i =0 ; i< stringList.length; i++){
                temp[i] = stringList[i];
            }

            stringList = temp;
            add(s);
        }
    }

    public String toString(){
        char[] output = new char[characterCount];
        int outputIndex = 0;

        for(int i = 0; i < size; i++){
            for(int j = 0; j < stringList[i].length(); j++){
                output[outputIndex++] = stringList[i].charAt(j);
            }
        }

        return new String(output);

    }

}

Test Class

package stringBuilder;

import java.util.Scanner;

public class Test {

    public static void main(String[] args){
        NoksStringBuilder stringBuilder = new NoksStringBuilder();

        Scanner inputScanner = new Scanner(System.in);
        int i = 0;

        while(i != 3){

            printMenu();
            i = Integer.parseInt(inputScanner.nextLine());

            if(i == 1){
                System.out.println("Enter string");
                stringBuilder.add(inputScanner.nextLine());
            }
            else if( i == 2){
                System.out.println(stringBuilder.toString());
            }

        }


    }

    private static void printMenu() {
        System.out.println("StringBuilder Options :");
        System.out.println("1.Add");
        System.out.println("2.ToString");
        System.out.println("3.Quit");
    }



}
share|improve this question
    
In an interview situation, as on Code Review, you should consider all code to be subject to review. =) – 200_success yesterday
    
Fair, i will edit my question.. :) – nikel yesterday

size and characterCount should be private. I suggest renaming them to stringCount and charCount, respectively. In the comments, "sum total" is redundant, as is "currently".

add() should not need to call itself. Just reorder the statements.

public void add(String s) {
    if (need expansion) {
        expand array
    }
    store s
}

When expanding the array, use System.arraycopy() — it's more succinct and slightly faster than a Java loop.

share|improve this answer
public class NoksStringBuilder {

When told that something implements StringBuilder, I would expect it to implement the same interfaces.

public class NoksStringBuilder implements CharSequence, Appendable, Serializable {

This way it can be used as a replacement for a StringBuilder used in more generic circumstances.

    String[] stringList = new String[INITIAL_SIZE];

You call this a stringList, but it's actually a String array. If it were an actual List, you could quit mucking around managing capacity.

      List<String> strings = new ArrayList<>();

This would push all the memory management off onto the List. But I actually don't think that anything involving String is correct here. More on that later.

    public void add(String s){

Similarly, I would call this

    public Appendable append(CharSequence csq) {

Then you could use it the same way as with a real StringBuilder. Or you could use it the way you do your method (realizing that a String is a CharSequence).

All this would make obvious that an array of String is not a good way to hold a StringBuilder. For one thing, while it makes for a quick append of a String, it works less well with letters and you lose the random access aspect. I'd probably try

    private char[] sequence = new char[DEFAULT_CAPACITY];

That puts more work into add but makes toString simpler.

    public String toString() {
        return new String(sequence, 0, length);
    }

Note that length is an object field that you'll have to maintain. It's the equivalent of your characterCount.

Note that if you want to implement capacity(), you could just say

    public int capacity() {
        return sequence.length;
    }
share|improve this answer
String[] temp = new String[stringList.length*2];

for(int i =0 ; i< stringList.length; i++){
    temp[i] = stringList[i];
}

I feel this is the major flaw in your algorithm.

You'd be better off with a linked list - no copying needed to expand the list.

Also, why are you manually copying from the string array when there is System.arraycopy to do just this for you? (Granted, I wouldn't use it in an interview either, but that's because the interviews I envision don't give me access to the internet or documentation, and getting the parameters for System.arraycopy right is tricky.)

The working of your StringBuilder is correct, though it doesn't offer much in terms of utility. It's not null safe, it's not thread-safe, and it doesn't offer any functionality besides appending Strings. There are no constructors that start with a string or capacity, and add returns void, so you can't chain method calls.

I expect a StringBuilder like this to end up in a utilities package so it is not a major concern, but your instance variables are package-scoped. So other code can violate your internals.

There's also a lack of an API; something which is much needed in utility classes because they're far more likely to be reused.

share|improve this answer

If you add a increaseCapacity() function your code gets easier to read:

public void add(String s){
    if(size < stringList.length){
        stringList[size++] = s;
        characterCount += s.length();
    }
    else{
        increaseCapacity();
        add(s);
    }
}

Now it's also easier to see that this can be written nicer by inverting the if:

public void add(String s){
    if(size >= stringList.length){
        increaseCapacity();            
    }

    stringList[size++] = s;
    characterCount += s.length();
}

This also got rid of the recursive call.

Misc

  • I doubt that your class performs better than simply using + to concatenate strings, but I'm assuming it's just for academic purposes.
  • Use unit tests to test, it's a lot easier than reading input from the command line.
  • initial capacity isn't such a helpful comment, as it is just a rephrasing of the variable name; if you think capacity is a better term, just rename the variable.
  • size isn't really the size of anything, but a count, so I would rename it to stringCount (otherwise readers may assume that it is the size of stringList; count also fits in better with characterCount).
  • your spacing is off (you can use any IDE to fix all formatting issues).
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.