6
\$\begingroup\$

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");
    }



}
\$\endgroup\$
  • \$\begingroup\$ In an interview situation, as on Code Review, you should consider all code to be subject to review. =) \$\endgroup\$ – 200_success Jun 25 '16 at 14:42
  • \$\begingroup\$ Fair, i will edit my question.. :) \$\endgroup\$ – nikel Jun 25 '16 at 14:47
10
\$\begingroup\$
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;
    }
\$\endgroup\$
  • \$\begingroup\$ Nice catch to say that i can use a list instead of an array, i was working on ArrayList & StringBuilders & I ended up mixing both :) Regarding implementing the standard interfaces , i agree that its helpful ; but i really wonder if a Java developer should be expected to know these. Lastly, char[] won't work because we need a list of strings, unless you meant something else :) \$\endgroup\$ – nikel Jun 27 '16 at 13:20
  • 1
    \$\begingroup\$ Your question doesn't say anything about needing a list of strings. If that's a requirement, then you should have included both that and any other requirements in your question. As is, your requirement is "alternative to StringBuilder" and a collection of String is a poor way to implement something described only as an alternative to StringBuilder. Note that I'd probably mention that as a criticism of the requirement if the requirement were included. \$\endgroup\$ – mdfst13 Jun 27 '16 at 15:00
  • \$\begingroup\$ Got your point. But why do you say that we loose the random access aspect with Strings? It internally has an array only. \$\endgroup\$ – nikel Jul 5 '16 at 1:23
  • 1
    \$\begingroup\$ @nikel If I want the 117th character of the NoksStringBuilder, I have to iterate through the strings to find the one that contains the 117th character. Each string itself is random access, but we don't know what string contains a particular character index. \$\endgroup\$ – mdfst13 Jul 5 '16 at 2:13
8
\$\begingroup\$

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.

\$\endgroup\$
5
\$\begingroup\$
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.

\$\endgroup\$
  • \$\begingroup\$ its a string builder , so no need to be thread-safe ;). nice point regarding chaining. A question here, why do you say it does not have an API? Agreed there are no constructor with string/capacity. I have an add() method which might make a very small API..? \$\endgroup\$ – nikel Jul 5 '16 at 1:28
  • \$\begingroup\$ @nikel there is no documentation, add and toString is only the minimal code needed for the object to actually work - it doesn't have ways to easily use the object. Contrast the standard StringBuilder implementation which will tell you a lot of stuff about how to use it. \$\endgroup\$ – Pimgd Jul 5 '16 at 7:52
5
\$\begingroup\$

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).
\$\endgroup\$
  • \$\begingroup\$ Since Java uses a StringBuilder to implement using + to concatenate strings, I think your first point is a given. \$\endgroup\$ – David Conrad Jun 30 '16 at 19:39
  • \$\begingroup\$ Great point regarding using Unit tests to test, reusable & improves unit testing skills! \$\endgroup\$ – nikel Jul 5 '16 at 1:33
1
\$\begingroup\$

In addition to the other feedback you've gotten, I wanted to comment on this:

package stringBuilder;

Java recommends using an inverted domain name such as com.example.something to avoid collisions between package names. I see a lot of code on here like this that I assume (no offense intended) is from beginners that just makes up an arbitrary package name, and I think that's a bad idea. It only costs about $5 - $10 to get your own domain name, but if you don't want to do that you can either use a package name based on a domain you're affiliated with (such as your educational institution, although that wouldn't guarantee no collisions) or you can just leave out the package declaration and leave your class in the unnamed, anonymous package.

\$\endgroup\$
  • \$\begingroup\$ Nice point. Only that this should ultimately end up in a utility library & if i use an anonymous package - nobody would be able to use it \$\endgroup\$ – nikel Jul 5 '16 at 1:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.