7
\$\begingroup\$

I want to concat two strings, by sub-stringing each one with same length, and concatenation should happen like I mentioned below

for suppose, I have str1 = raines, str2 = bowls, and LENGTH as 2, then my output should be raboinwless.

edit: starting from leftmost alphabet, identify LENGTH number of letters from str1 and str2, and add the identified letters to output string respectively. repeat this until either str1 or str2 or both have less than LENGTH letters left. Add the remaining letters (if any) from str1 followed by remaining letter (if any) from str2 to output string.

(first two letters from str1 is 'ra', and 'bo' from str2, concatenating them gives 'rabo', in next cycle the next two letters are 'in' from str1 and 'wl' from str2 by concatenating we'll get 'inwl', the total output now is 'raboinwl', at last we now have 'es' from str1, 's' from str2, the overall output is 'raboinwless')

import java.util.Scanner;

public class practice 
{   
    //input str1 = raines
    // str2 = bowls
    // num = 2
    //expected output = raboinwless
    public static void main(String[] args) 
    {        
        Scanner input = new Scanner(System.in);
        String instr1 = input.nextLine();
        String instr2 = input.nextLine();
        int innum = input.nextInt();
        String outstr="";       
        if(innum > instr1.length() || innum > instr2.length()) {
                outstr = instr1+instr2;
                System.out.println(outstr);
        }else if(innum < instr1.length() || innum < instr2.length()) {
            String[] chunks1 = instr1.split("(?<=\\G.{" + innum + "})");
            String[] chunks2 = instr2.split("(?<=\\G.{" + innum + "})");            
            int itrCount = 0;
            if(chunks1.length > chunks2.length)
                itrCount = chunks1.length;
            else itrCount = chunks2.length;             
                        
                for(int i= 0 ; i< itrCount;i++) {
                    outstr = outstr+chunks1[i];
                    outstr = outstr+chunks2[i];                 
                }
            System.out.println(outstr);
        }
    } 
}
  • if I'm not making any sense or my way the questioning is bad, please comment or restructure my question,
\$\endgroup\$
1
  • \$\begingroup\$ I wonder if "striping" the strings is the right phrase to describe what you're doing (like with RAID0 disk striping). \$\endgroup\$ Feb 10, 2022 at 5:00

2 Answers 2

6
\$\begingroup\$

Welcome to code review! I'm not doing a conventional review where I would go through your code and point out things that could be improved, because frankly, that would cover just about everything in it. Instead I'll show you how I would approach the problem.

Instead of putting all your code in the main method, write a minimal method that performs the task ou have been given. In your case, it would be a method that takes two strings as parameters and returns the spliced string.

public static String splice(String str1, String str2, int chunkSize) {
    // We will implement this later.
    return null;
}

Then you can call this method from your main method with whatever corner case parameters you can imagine and print out the result (later in your studies you will learn about unit testing, this is an extremely rudimentary form of it). Forget the Scanner. The task did not require it and it makes testing the code really difficult.

public static void main(String[] args) {
    System.out.println(splice("raines", "bowls", 2));
    System.out.println(splice("foo", "bar", 1));

    // Did you test your code with invalid input?
    System.out.println(splice("", "", 0));

    // Always test with null values.
    System.out.println(splice(null, null, 1));

    // Can the code handle extreme values?
    System.out.println(splice("a", "b", Integer.MAX_VALUE));
}

Then, always try to first find out if there exists utilities in the standard library that would make your job easier. Iterating over characters in a string seems like a task that many people have faced before you. So looking for something to help in that reveals the StringCharacterIterator. I didn't even know it existed before this. I used an internet search engine...

Your task is to take characters from one string and stuff them into a result. Then take the same amount of characters from another string and stuff them into the same result. And repeat. Repeating the same thing always suggests that a method should be made. Since the StringCharacterIterator handles keeping track of the current postion, the method becomes quite simple: if there are characters left in the string, move one to the result and advance to next character. Repeat until count has been reached or string has been exhausted.

private static void moveCharacters(CharacterIterator from, StringBuilder to, int count) {
    for (int i = 0; i < count; i++) {
        final char ch = from.current();
        if (ch == CharacterIterator.DONE) {
            return;
        }
        to.append(ch);
        from.next();
    }
}

And the method now becomes:

public static String splice(String str1, String str2, int chunkSize) {
    if (chunkSize < 1) {
        throw new IllegalArgumentException("Chunk size must be greater than zero");
    }

    final CharacterIterator iter1 = new StringCharacterIterator(str1);
    final CharacterIterator iter2 = new StringCharacterIterator(str2);
    final StringBuilder result = new StringBuilder(str1.length() + str2.length());
    while (iter1.current() != CharacterIterator.DONE || iter2.current() != CharacterIterator.DONE) {
        moveCharacters(iter1, result, chunkSize);
        moveCharacters(iter2, result, chunkSize);
    }
    return result.toString();
}

The main points are: identify the core of your problem (the public method) and split it into easily manageable tasks (the private sub-methods).

\$\endgroup\$
1
  • \$\begingroup\$ Thank you very much, that a lot to digest, will follow the suggestions you've provided. and will update my answer. thanks a lot. \$\endgroup\$
    – raj
    Feb 11, 2022 at 14:05
3
\$\begingroup\$

In addition to Torben's answer, that points you to a better solution architecture, I'll go the "conventional" way of commenting on your code.

Package

Java code should avoid the unnamed package, but instead be placed into a unique package, one that most prabably never collides with any other Java code. The convention is to use some internet domain or id that "belongs" to you, e.g.

package com.stackexchange.codereview.raj.slicer;

Class naming

A class name should reflect its purpose. And Java naming convention is to begin class names with uppercase letters. So, a better name would be

public class SlicedConcatenator

Formatting

Your code formatting is inconsistent. Sometimes the opening { goes to the same line, sometimes on a line of its own. Typical Java style is to have it at the end of the same line.

Your indentation is inconsistent. Most of the time, it's 4 spaces, but sometimes 8. Typical style is 4 spaces.

        else itrCount = chunks2.length;             
                    
            for(int i= 0 ; i< itrCount;i++) {
                outstr = outstr+chunks1[i];
                outstr = outstr+chunks2[i];                 
            }
        System.out.println(outstr);

Here you have an indentation mistake. The for loop is at the same logical level as the else and the System.out.println, but is indented differently.

Most of the time, you surround operators like = with spaces (good), but sometimes it's only one space, after the operator.

Nowadays, there's no need to format code yourself, every decent IDE or code editor can do it for you automatically. Find this feature in your development environment and use it.

Variable naming

Variable names should describe the meaning of the contents. While instr1, instr2 and outstr are quite understandable, innum could better be sliceLength.

Use curly braces

        else itrCount = chunks2.length;             
                    
            for(int i= 0 ; i< itrCount;i++) {
                outstr = outstr+chunks1[i];
                outstr = outstr+chunks2[i];                 
            }

This is a classical formatting problem. Judging by indentation, it looks as if the for loop still is part of the else branch. Always use { } braces around blocks, even if they only consist of a single instruction:

        if (chunks1.length > chunks2.length) {
            itrCount = chunks1.length;
        } else {
            itrCount = chunks2.length;
        }           
                    
        for (int i = 0 ; i < itrCount; i++) {
            outstr = outstr + chunks1[i];
            outstr = outstr + chunks2[i];                 
        }

Program logic

        if(innum > instr1.length() || innum > instr2.length()) {
            outstr = instr1+instr2;

What if instr1 == "averylonginputstring" and instr2 == "short" and innum == 8? You'll get "averylonginputstringshort", but as far as I understand the task, the result should be "averylonshortginputstring". I'd apply the simplified case only if both strings aren't longer than the slice length:

        if(innum >= instr1.length() && innum >= instr2.length()) {

Note the >= instead of > (a 4-character string with a slice length of 4 is still a simple case), as well as the && instead of the || (AND combination instead of OR).

You idea, to identify cases that can be solved more easily than the general case, is good, but you have to be very careful when to take such a shortcut. And often it's even better to ignore the shortcut and always use the general solution path.

Your two cases

    if(innum > instr1.length() || innum > instr2.length()) {
        // ...
    }else if(innum < instr1.length() || innum < instr2.length()) {
        // ...
    }

leave a loophole. What if instr1 == "test" and instr2 == "test" and innum == 4? None of the conditions match, and you end up with an empty result. In situations like yours, where you want one algorithm to get used, make sure you have one unconditional else part:

    if(...) {
        // ...
    }else {
        // ...
    }

The lengthy snippet

        int itrCount = 0;
        if(chunks1.length > chunks2.length)
            itrCount = chunks1.length;
        else itrCount = chunks2.length;

can be rewritten as

        int itrCount = Math.max(chunks1.length, chunks2.length);

which makes an inherent problem obvious: you correctly understood that the two strings don't necessarily produce the same number of chunks, but how does your code deal with that situation? E.g. instr1 == "testtesttesttest" and instr2 == "test" and innum == 2? You'll get an ArrayIndexOutOfBoundsException instead of the correct answer.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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