4
\$\begingroup\$

Found this problem somewhere online and wanted to try this out with my beginner skills. I was able to solve this problem but how can I make my code better?

We need to print a pattern like this (each line has one more character in length):

E  
my  
iyt  
hTri  
bbiann  
i

Here is the solution I have written, and I want help in the following areas:

What am I doing wrong and how can I make it better?
How can I learn to write better code and logic?

public class nameIncrementing {

    public static void main(String[] args) {




                char[] text={'E','m','y','i','y','t','h','T','r','i','b','b','i','a','n','n','i'};
                int count =0;
                int length = text.length;
                int icnt=0;
                for(int i=0;i<length;i++){
                    if(i==0)
                        System.out.println(text[0]);
                    else{
                        if(count+1>=length)
                        {break;}
                        else{
                            for(int j=count+1;j<i+2+count;j++){
                                icnt++;
                                if(j>=length)
                                    break;
                                else
                                    System.out.print(text[j]);                              
                            }
                    count = count+icnt;
                    icnt=0;
                    }

                    System.out.println();}
                }
    }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ I read you edit suggestion but wouldn't be E my iyt hTri bbian ni the expected result? Can you include the link where you got that challenge? Could you paste the the description of the challenge on your question? \$\endgroup\$ – Bruno Costa Jun 20 '16 at 8:42
6
\$\begingroup\$

First step - make your current code better

Instead of a char array you might prefer to use a String. This would make the declaration easier:

String text = "EmyiythTribbianni";

I would say that most of us generally recommend that you shouldn't omit brackets. You should also follow a consistent bracket pattern.

I recommend for every statment to be in a different line from your brackets. This means that things like {break;} are not really that good.

Closing brackets should also be matching the same identation of the statment they were open. By this I mean

ìf(condition){
    //code goes here
    for(declaration; evaluation; increment){
        //code goes here 
    }//closing bracket with the same indentation as the previous for statmeent
    //code goes here
}//closing bracket with the same indentation as the previous if statement

You should also follow a consistent spacing and let prefer more space than less.

  • This int count =0; becomes int count = 0;
  • This for(int i=0;i<length;i++) becomes for(int i = 0; i < length; i++)
  • This if(j>=length) becomes if(j >= length)

I think you get the ideia. Every time you find a comparison, assignment, +, -, or ; (on for) you space it.

Your code would then become something like this

String text = "EmyiythTribbianni";
int count = 0;
int length = text.length;
int icnt = 0;
for(int i = 0; i < length; i++){
    if(i == 0){
        System.out.println(text[0]);
    }else{
        if(count + 1 >= length){
            break;
        }else{
            for(int j = count + 1; j < i + 2 + count; j++){
                icnt++;
                if(j >= length){
                    break;
                }else{
                    System.out.print(text[j]);    
                }                   
            }
            count = count + icnt;
            icnt = 0;
        }
        System.out.println();
    }
}

Second step - taking a different approach

It seems that you want to put according to the number of character that you had printed so far. I reckon that the arithmetic progression sum formula n(a1 + an) / 2 would be useful here.

The first time you print 1 character making the first term a1 = 1. Then you print a space. The second time you print 2 characters a2 = 2, the total of characters print to this time is 2 * (1 + 2) / 2 = 3. And so on.

By applying this idea you would get something along this lines:

String text = "EmyiythTribbianni";
int n = 1;

for(int i = 0; i < length; i++){
    int sum = n  * (1 + n) / 2;
    if(i != sum){
        System.out.print(text[i]);
    }else{
        System.out.print(" ");
        System.out.print(text[i]);
        ++n;
    } 
}
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ In javathe String indexing syntax doesn't exist does you should use text.get(i) instead. \$\endgroup\$ – Bruno Costa Jun 20 '16 at 9:35
3
\$\begingroup\$
                char[] text={'E','m','y','i','y','t','h','T','r','i','b','b','i','a','n','n','i'};

You can write this more easily as

        char[] text = "EmyiythTribbianni".toCharArray();

Even with the extra method call, this is still shorter. It's also easier to type.

Your indentation is a bit off. I reduced it down to what I'd expect: one four-space indent from the enclosing method definition.

            int count =0;
            int length = text.length;
            int icnt=0;

I'm still not sure what count and icnt represent. Such naming issues can be a sign of unnecessarily complicated logic.

Consider instead

        StringBuilder line = new StringBuilder();
        int charactersPerLineMaximum = 1;
        for (char c : text) {
            line.append(c);

            if (line.length() >= charactersPerLineMaximum) {
                System.out.println(line);
                charactersPerLineMaximum++;
                line.setLength(0);
            }
        }

        if (line.length() > 0) {
            System.out.println(line);
        }

Hopefully charactersPerLineMaximum is clear. When the line length reaches the maximum, we print the current line, start a new line, and increase the maximum. This produces the same output as your version, and I find this easier to follow.

Even shorter

        String text = "EmyiythTribbianni";

        int charactersPerLineMaximum = 1;
        for (int i = 0; i < text.length(); i += charactersPerLineMaximum++) {
            System.out.println(text.substring(i, Math.min(i + charactersPerLineMaximum, text.length())));
        }

This may be more efficient as well. The downside being that its logic is relatively dense. It might be more readable as a while loop, which would unfold some of the logic.

Note that without the Math.min, it throws an exception because it tries to run past the end of the string.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

"How can I make it better?"

"How can I learn to write better code and logic?"

You have to know what's the difference between better and worse code. There are some general rules beginners have a at least some chance to produce ordinary code. But in real world projects even experts have problems to produce "good" code. As you are convinced that there is something like "better" or "worse" (I guess so) you are on the right way.

First of all you have to train algorithmic thinking.

After that you have to get a feeling what the general programming paradigm you are applying is all about (OO vs. functional programming). My suggestion is to start with only one paradigm to not be confused. I would favor OO in JAVA. But for training purposes take what you want.

After you master algorithmic thinking (you are able to "SOLVE" even complex problems) and you have a basic understanding of the programming paradigm structuring code is the next step. You can slowly go into clean code as a set of best practises in programming. This will take time if you are serious.

Passing every level of clean code you should study the design patterns of the GoF.

Having a look at literature for object oriented analysis and design could be helpful.

Then you should go into architecture and master some technologies in every layer (ui, backend, persistence).

Finally the SOLID principles together with the "law of demeter" are the "Holy Grail". Applying them you are able to improve your code in every iteration you identify violations of SOLID. They will also drive architecture and you will find out the exceptions of every best practise.

Now to your code:

The first idea is that your code should not hide its intention. Somehow you should mention that...

  1. you have an initial length of a name
  2. you have a char sequence from where you take chars from the beginning with the given length
  3. the amount of chars you want to take is incremented by 1 each turn
  4. you want to take chars until there are no chars anymore

Everything else should be subordinated and follow this semantic because this is what you want to express.

The artifacts expressed above formulate the first responsibility (SRP).

So my suggestion for the top level code:

public class NameIncrementing {


    private static final int INITIAL_NAME_LENGTH = 1;


    public static void main(String[] args) {

        CharSequence charSequence = new CharSequence("EmyiythTribbianni");

        int nameLength = INITIAL_NAME_LENGTH;

        while (charSequence.hasMoreChars()) {

            StringBuffer name = charSequence.takeNextChars(nameLength);

            System.out.println(name);

            nameLength++;

        }

    }


}

The second responsibility is a char sequence. CharSequence in this case is a class that wraps a StringBuffer to fit the needs of taking chars from the beginning.

public class CharSequence {

    private StringBuffer sequence;

    public CharSequence(String sequence) {
        this.sequence = new StringBuffer(sequence);
    }

    public boolean hasMoreChars() {
        return sequence.length() > 0;
    }

    public StringBuffer takeNextChars(int amount) {

        StringBuffer nextChars = new StringBuffer();

        for (int count = 0; count < amount && hasMoreChars(); count++) {
            nextChars.append(sequence.charAt(0));
            sequence.deleteCharAt(0);
        }

        return nextChars;
    }

}

The trick is to extend the capabilities of StringBuffer by "composition" to shorten the way to your goal. By the way StringBuffer is final so you cannot derive from it.

StringBuffer has no method to ask "hasMoreChars". So you encapsulate "sequence.length() > 0".

StringBuffer has no method to take the first X chars so you formulate "takeNextChars(amount)" with the given capabilities.

The pattern used here is an unfinished "decorator pattern". As StringBuffer is final there is a little bit more work to do to finish it but I think this solution is proper to the task and can be extended without blocking.

One side note: In my opinion the primary task of a developer is not to think about what you can do... the primary task is about to omit things without obstruct possibilities but meet the requirements (related to the Open-Closed-Principle).

| improve this answer | |
\$\endgroup\$

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.