1
\$\begingroup\$

I have executed a program which reads long string and divide the string on basis of specified input. Well I am getting the correct output but would try it to be more efficient and readable. Opinions and tactics are welcomed to make the code more better and fast.

Problem Statement:: Need to split the input string on basis of input parameter.In given input string First parameter length is fixed(e.g.given string Starts with TUEF having fixed length of 94).Other parameter having variable in length(e.g.Given string next input for split is "PN03N01" which is variable in length.Next input parameter is "ID03I" which is also variable in length and it can appear in many times in string like"ID03I01","ID03I02"..)

Solution:: Added all input code in stack and removed the required data against input string and put into map using recursive way

Below is code::

import java.util.HashMap;
import java.util.Iterator;
import java.util.Set;
import java.util.Stack;

public class TUEFSplitter {


    public static void main(String[] args) {
        HashMap hm  = new HashMap();
        String str = "TUEF12111492911                  0000BP03251021                    100135514576830122016170412PN03N010125NAMEXXXX MIDDLEN LASTNAM0225NAMEXXXX MIDDLEN LASTNAM07082810198108012ID03I010102010210AFFPT8319BID03I0201020602126165040199789001YPT03T010108425188000302039001YPT03T02011022425188000302029001YPT03T03010842518800030203PT03T04011099877939000302019001YEM03E01010210020831102016030201SC10CIBILTUSC2010204020210030830122016040500694250220260211270205PA03A010124610  B WING   SAGAR TECH0224PLAZA ANDHERI KURLA ROAD0323SAKI NAKA JUNCTION NEAR0422BY SAKI NAKA METRO STN0506MUMBAI06022707064000720802031008280620169001YPA03A020122501-B2 EKTA SAFALE CHS0220OLD AGRA ROAD CASTLE0309NILL NAKA0505THANE0602270706400601080204100824062016PA03A030115SAKINAKA MUMBAI06022707064000700802031008310320169001YPA03A0401362B 501 EKATA SAFALE CHS BH. SAIANAND0206MUMBAI06022707064001640802021008310320169001YTL04T0010213NOT DISCLOSED040210050110808230320160908301020161108301120161205272231305272232827000000000000000000000000000300801112016310801032016IQ04I0010108281220160413NOT DISCLOSED0502050606900000IQ04I0020108231220160413NOT DISCLOSED0502050606400000IQ04I0030108251120160413NOT DISCLOSED050210060550000IQ04I0040108260820160413NOT DISCLOSED050210060550000IQ04I0050108120820160413NOT DISCLOSED0502100606100000IQ04I0060108120720160413NOT DISCLOSED050210060550000IQ04I0070108280620160413NOT DISCLOSED050210060550000IQ04I0080108240620160408RBL BANK05021006041000IQ04I0090108310320160413NOT DISCLOSED050210060550000IQ04I0100108200320160413NOT DISCLOSED05021006041000IQ04I0110108010120160413NOT DISCLOSED050210060515000ES0700016210102**";
        hm.put(str.substring(0,4),str.substring(4,94));
        //System.out.println("original String length is " +str.length());   
        Stack st = new Stack();
        st.push("ES07");
        st.push("DR03D0");
        st.push("IQ04I0");
        st.push("TL04T0");
        st.push("PA03A");
        st.push("SC10CIBILTUSC2");
        st.push("PI03I");
        st.push("EM03E");
        st.push("EC03C");
        st.push("PT03T");
        st.push("ID03I");
        spillter(str.substring(94),hm ,st);
        System.out.println("Size of HashMap is " +hm.size());
        Set set = hm.keySet();
        Iterator it =set.iterator();
        while (it.hasNext()) {
            String key = (String) it.next();
        System.out.println("Keyyyy is " +key);
        System.out.println("Value is "+hm.get(key));
        }
    }

    static String startPoint = "PN03N01";
    static void spillter(String str ,HashMap hm , Stack st)
    {
        if(str.contains(startPoint)){           
            int i= 1;
            String searchString;
            if(st.size()>0)
            {
                 searchString = st.pop().toString();    
            }
            else
            {
                searchString = startPoint;
            }

            while(true)
               {        
                   String fsearch;
                     if(searchString.equals("SC10CIBILTUSC2") ||searchString.equals("ES07") )
                     {
                         fsearch = searchString;
                     }
                     else                        
                     {
                         if(i<=9)
                         {
                             fsearch = searchString+"0"+i; 
                         }
                         else
                         {
                             fsearch = searchString+i;   
                         }

                     }
                  if(str.contains(fsearch))
                   {
                       int end = str.indexOf(fsearch);  
                       if(end == 0)
                       {
                           end = str.length();
                       }
                       hm.put(startPoint,str.substring(startPoint.length(), end));                     
                       str = str.substring(end);
                       startPoint = fsearch;
                       i++;
                       if(fsearch.equals("SC10CIBILTUSC2"))                       
                           {
                           spillter(str,hm,st); 
                           break;
                           }                       
                   }
                   else
                   {
                       if(str.length()>0 && !str.isEmpty())
                       {
                           spillter(str,hm,st);                       
                       }                         
                       break;
                   }
               }
            return ;
            }
        else
        {
            return ;
        }

    }
}
Improve this question
\$\endgroup\$

4 Answers 4

Reset to default
2
\$\begingroup\$

First of all, you should fix your indentation. Get and use an IDE - they have auto-formatters which you can use if you have trouble getting things indented properly.

For an example, 

    while (it.hasNext()) {
        String key = (String) it.next();
    System.out.println("Keyyyy is " +key);
    System.out.println("Value is "+hm.get(key));
    }

This is misleading.

It looks like it would function like

    while (it.hasNext())
        String key = (String) it.next();

    System.out.println("Keyyyy is " +key);
    System.out.println("Value is "+hm.get(key));

Or like

    while (it.hasNext()) {
        String key = (String) it.next();
    }

    System.out.println("Keyyyy is " +key);
    System.out.println("Value is "+hm.get(key));

But in-fact it's just

    while (it.hasNext()) {
        String key = (String) it.next();
        System.out.println("Keyyyy is " +key);
        System.out.println("Value is "+hm.get(key));
    }

Like that. Keep your indentation consistent to avoid surprises.

Iterating over a hashmap is usually done with .entries: 

    System.out.println("Size of HashMap is " +hm.size());
    Set set = hm.keySet();
    Iterator it =set.iterator();
    while (it.hasNext()) {
        String key = (String) it.next();
        System.out.println("Keyyyy is " +key);
        System.out.println("Value is "+hm.get(key));
    }

That's your code. If we replace this with a for entry in entries loop...

System.out.println("Size of HashMap is " +hm.size());
for (Map.Entry<String, String> entry : hm.entries()) {
    System.out.println("Keyyyy is " +entry.getKey());
    System.out.println("Value is "+entry.getValue());
}

As you can see, this shortens the code. It even provides nice getKey and getValue methods instead of making you iterate through the keys and then having to find the corresponding value. 

        return ;
        }
    else
    {
        return ;
    }

This looks suspicious.

When I cut out the body of your if-statements, I get this:

static void spillter(String str ,HashMap hm , Stack st)
{
    if(str.contains(startPoint)) {           
        int i= 1;
        String searchString;
        if(st.size()>0)
        {
             searchString = st.pop().toString();    
        }
        else
        {
            searchString = startPoint;
        }

        while(true)
        {        
           String fsearch;
           if(searchString.equals("SC10CIBILTUSC2") ||searchString.equals("ES07") )
           {
               fsearch = searchString;
           }
           else                        
           {
               if(i<=9)
               {
                   fsearch = searchString+"0"+i; 
               }
               else
               {
                   fsearch = searchString+i;   
               }
           }
           if(str.contains(fsearch))
           {
                //codes
                if(fsearch.equals("SC10CIBILTUSC2"))                       
                {
                    //codes
                    break;
                }                       
            }
            else
            {
                //codes                      
                break;
            }
        }
        return ;
    }
    else
    {
        return ;
    }
}

There's a couple things to notice:

    if(str.contains(startPoint)) {           
        //initialization code
        while(true)
        {        
           //code containing break statements
        }
        return ;
    }
    else
    {
        return ;
    }

You have two cases of an if-statement (the true and the false case) and they both end with the same line - return ;. In that case, we can move this line out of the if-statement, since it needs to be executed regardless of true or false.

static void spillter(String str ,HashMap hm , Stack st)
{
    if(str.contains(startPoint)) {           
        //initialization code
        while(true)
        {        
           //code containing break statements
        }
    }
    else
    {
    }
    return;
}

But now we have an empty else statement, so that should go.

static void spillter(String str ,HashMap hm , Stack st)
{
    if(str.contains(startPoint)) {           
        //initialization code
        while(true)
        {        
           //code containing break statements
        }
    }
    return;
}

Lastly, it makes no sense to have a return statement at the end of a function which will return void. So we can remove the return statement.

static void spillter(String str ,HashMap hm , Stack st)
{
    if(str.contains(startPoint)) {           
        //initialization code
        while(true)
        {        
           //code containing break statements
        }
    }
}

The other thing I noticed is that you have an else statement containing only an if-else chain.

           if(searchString.equals("SC10CIBILTUSC2") ||searchString.equals("ES07") )
           {
               fsearch = searchString;
           }
           else                        
           {
               if(i<=9)
               {
                   fsearch = searchString+"0"+i; 
               }
               else
               {
                   fsearch = searchString+i;   
               }
           }

This section.

When you have such a structure, you can make use of else if:

           if(searchString.equals("SC10CIBILTUSC2") ||searchString.equals("ES07") )
           {
               fsearch = searchString;
           }
           else if(i<=9)
           {
               fsearch = searchString+"0"+i; 
           }
           else
           {
               fsearch = searchString+i;   
           }

This simplifies your code and prevents you from going really far with your indentation.

Alright, so here's your code now, for the spillter function.

static void spillter(String str ,HashMap hm , Stack st)
{
    if(str.contains(startPoint)) {           
        int i = 1;
        String searchString;
        if(st.size()>0)
        {
            searchString = st.pop().toString();    
        }
        else
        {
            searchString = startPoint;
        }

        while(true)
        {        
            String fsearch;
            if(searchString.equals("SC10CIBILTUSC2") ||searchString.equals("ES07") )
            {
                fsearch = searchString;
            }
            else if(i<=9)
            {
                fsearch = searchString+"0"+i; 
            }
            else
            {
                fsearch = searchString+i;   
            }
            if(str.contains(fsearch))
            {
                int end = str.indexOf(fsearch);  
                if(end == 0)
                {
                    end = str.length();
                }
                hm.put(startPoint,str.substring(startPoint.length(), end));                     
                str = str.substring(end);
                startPoint = fsearch;
                i++;
                if(fsearch.equals("SC10CIBILTUSC2"))                       
                {
                    spillter(str,hm,st); 
                    break;
                }                       
            }
            else
            {
                if(str.length()>0 && !str.isEmpty())
                {
                    spillter(str,hm,st);                       
                }                         
                break;
            }
        }
    }
}

There's some more improvements I can see:

if(st.size()>0)

You can use !st.empty().

if(str.length()>0 && !str.isEmpty())

str.isEmpty() already checks that length == 0 internally, so basically, if the first condition is true, the second part will also always be true. I'd replace it with if (!str.isEmpty()) because that's what you're going for.

Improve this answer
\$\endgroup\$
2
\$\begingroup\$

Since this is codereview and your code is really hard to read, I'm first going to fix the style before even looking at the algorithm.

Use Generics

If you know that a map is mapping a String to another String then you write:

HashMap<String, String> hm = new HashMap<>();

Same for the Stack of Strings:

Stack<String> st = new Stack<>();

This also makes the toString redundant in the following line:

searchString = st.pop().toString();

The compiler already knows that st.pop() returns a String. So this simple becomes:

searchString = st.pop();

foreach instead of while

With the above change we can greatly simplify doing something for each item in the keyset. Instead of writing:

Set set = hm.keySet();
Iterator it = set.iterator();
while (it.hasNext()) {
    String key = (String) it.next();

We can write this one line:

for (String key : hm.keySet()) {

My IDE is still complaining that it's not efficient to first loop over each key and then search for the value in the map again, instead of getting both key and value at the same time. It wants to replace the for loop with this:

for (Map.Entry<String, String> entry : hm.entrySet()) {
    System.out.println("Keyyyy is " + entry.getKey());
    System.out.println("Value is " + entry.getValue());
}

This is more efficient, but slightly less readable, so your choice if you want to do this last step or not.

package private

It's better to keep visibility to a minimum. Especially if you or someone else wants to change the signature of a method. If it's private you can be certain that it will only be used inside that file. Otherwise you need to check the entire package or even project to make sure you don't brake anything.

So reduce the visibility of the spillter(..) method and startPoint variable to private.

return early

Instead of using this construction:

if(condition) {
    //main work
} else {
   return;
}

We can swap this around and do

if(!condition) {
     return;
}

//main code

Which is easier to follow and saves us a level of indentation.

Also for your

while(true) {
    // do a lot
    if(..){
        break;
    }
}
return;

Why not return at the point of a break? This makes it easier to see that those are the points where you actually return. And there's no confusion about that weird return statement at the end of an endless while(true) loop.

I would also flip the if(str.contains(fsearch)) to make it easier to follow (and again save a level of indentation by removing the explicit else)

    if (!str.contains(fsearch)) {
        if (!str.isEmpty()) {
            spillter(str, hm, st);
        }
        return;
    }

check empty

Instead of writing st.size() > 0 it's better to use st.isEmpty() (note this also flips the if/else in this case).

String searchString;
if (st.isEmpty()) {
    searchString = startPoint;
} else {
    searchString = st.pop().toString();
}

Especially funny on this line:

if (str.length() > 0 && !str.isEmpty()) {

Which is exactly the same as:

if (!str.isEmpty()) {

else if

you can combine 

} else {
   if(...) {

into 

} else if(...) {

to save a level of indentation again.

magic constants

For you the strings "SC10CIBILTUSC2"and "ES07" might actually mean something. But for us those are just random strings. You should put them into a constant with a meaningful name. Sadly I don't know what it is so I can't name it properly:

public static final String SC10SOMETHING = "SC10CIBILTUSC2";

constant.equals(var)

When checking for equals, it's safer to call it on the constant String instead of a possibly null variable. In this case it's really unlikely that it's going to be null but it's a good habit.

SC10SOMETHING.equals(searchString)

interface > concrete implementation

The only point where you should use the concrete implementation (for example HashMap is when you call it's constructor. Everywhere else you should use the interface Map. If you ever decide that instead of a HashMap you want it to be a TreeMap you only need to change it in 1 place. Where it matters. All other functions etc. stay unchanged because they only care that it's a Map. For example:

private static void spillter(String str, Map<String, String> hm, Stack<String> st) {

End result of the spillter method:

private static void spillter(String str, Map<String, String> hm, Stack<String> st) {
    if (!str.contains(startPoint)) {
        return;
    }

    int i = 1;
    String searchString;
    if (st.isEmpty()) {
        searchString = startPoint;
    } else {
        searchString = st.pop();
    }

    while (true) {
        String fsearch;
        if (SC10SOMETHING.equals(searchString) || "ES07".equals(searchString)) {
            fsearch = searchString;
        } else if (i <= 9) {
            fsearch = searchString + "0" + i;
        } else {
            fsearch = searchString + i;
        }

        if (!str.contains(fsearch)) {
            if (!str.isEmpty()) {
                spillter(str, hm, st);
            }
            return;
        }

        int end = str.indexOf(fsearch);
        if (end == 0) {
            end = str.length();
        }
        hm.put(startPoint, str.substring(startPoint.length(), end));
        str = str.substring(end);
        startPoint = fsearch;
        i++;
        if (SC10SOMETHING.equals(fsearch)) {
            spillter(str, hm, st);
            return;
        }
    }
}

This still looks a bit long, so let's extract some specific parts. Like deciding the value of fsearch. This looks like it belongs into it's own method:

private static String parseSearchString(int i, String searchString) {
    String fsearch;
    if (SC10SOMETHING.equals(searchString) || "ES07".equals(searchString)) {
        fsearch = searchString;
    } else if (i <= 9) {
        fsearch = searchString + "0" + i;
    } else {
        fsearch = searchString + i;
    }
    return fsearch;
}

At least now the code itself looks readable. But after all this refactoring I'm tired of looking at it. So I suggest you post a follow up question with the fixed code style where someone might actually get motivated to look at what the code actually does :)

Improve this answer
\$\endgroup\$
1
\$\begingroup\$

I'll try to give you some ideas how I would improve your code:

In general your names are not very readable. You called your stack st. But what values will it contain? Maybe searchTokenStack or something like that? You called a counter i. What kind of counter is this?

Try to fail fast. In your example on line 40 you check a precondition of your method. But the corresponding return statement is on line 101. You can improve this like that:

if(!str.contains(startPoint)) {
  return;
}
// rest of your code

This reduces the indention level of your code.

Try to use the most descriptive/readable methods: On line 43 you check st.size() > 0. It is the same like st.empty() but you don't have to think about the meaning.

You should try to put some code in other methods so you have a readable name instead of difficult code. For example:

Line 61-68: You want to have a two digit number. This is could be done in a separat method. There are many Threads here on SO how to do this in general, but your code is find here. Just extract this in a seperate method.

Line 71: you use contain which is the same as indexOf(str) > -1. But on line 73 you call indexOf with the same arguments and the same string. You can optimize this to

int end = str.indexOf(fsearch);
if (end > -1) { //...

Line 90: str.length()>0 && !str.isEmpty() is the same like !str.isEmpty().

Optimization often requires rewriting big junks of code. But I don't understand the algorithm at first sight. So I am not able to give you any tip for this.

Improve this answer
\$\endgroup\$
0
\$\begingroup\$

I found other solution to above problem. I solved it by using regex.

Below is code::

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class TestRegex {
    private static String testString = "THAKARE0225RAJENDRA JANKIRAM THAKARE07082810198108012ID03I010102010210AFFPT8319BID03I0201020602126165040199789001YPT03T010108425188000302039001YPT03T02011022425188000302029001YPT03T03010842518800030203PT03T04011099877939000302019001YEM03E01010210020831102016030201SC10CIBILTUSC2010204020210030830122016040500694250220260211270205PA03A010124610  B WING   SAGAR TECH0224PLAZA ANDHERI KURLA ROAD0323SAKI NAKA JUNCTION NEAR0422BY SAKI NAKA METRO STN0506MUMBAI06022707064000720802031008280620169001YPA03A020122501-B2 EKTA SAFALE CHS0220OLD AGRA ROAD CASTLE0309NILL NAKA0505THANE0602270706400601080204100824062016PA03A030115SAKINAKA MUMBAI06022707064000700802031008310320169001YPA03A0401362B 501 EKATA SAFALE CHS BH. SAIANAND0206MUMBAI06022707064001640802021008310320169001YTL04T0010213NOT DISCLOSED040210050110808230320160908301020161108301120161205272231305272232827000000000000000000000000000300801112016310801032016IQ04I0010108281220160413NOT DISCLOSED0502050606900000IQ04I0020108231220160413NOT DISCLOSED0502050606400000IQ04I0030108251120160413NOT DISCLOSED050210060550000IQ04I0040108260820160413NOT DISCLOSED050210060550000IQ04I0050108120820160413NOT DISCLOSED0502100606100000IQ04I0060108120720160413NOT DISCLOSED050210060550000IQ04I0070108280620160413NOT DISCLOSED050210060550000IQ04I0080108240620160408RBL BANK05021006041000IQ04I0090108310320160413NOT DISCLOSED050210060550000IQ04I0100108200320160413NOT DISCLOSED05021006041000IQ04I0110108010120160413NOT DISCLOSED050210060515000ES0700016210102**";
    private static HashMap<String, List<String>> parsedMap = new HashMap<String, List<String>>();
    public static void main(String[] args) {
        String regex = "(?=(((DR03D0|IQ04I0|TL04T0|PA03A|SC10CIBILTUSC2|SC10CIBILTUSC2|PI03I|EM03E|EC03C|PT03T|ID03I)(.*?|\n.*?)(ES07|DR03D0|IQ04I0|TL04T0|PA03A|SC10CIBILTUSC2|SC10CIBILTUSC2|PI03I|EM03E|EC03C|PT03T|ID03I))|((ES07)(.*))))";
        Pattern pattern = Pattern.compile(regex);
        Matcher matcher = pattern.matcher(testString);

        while(matcher.find()) {
            String key = matcher.group(3);
            String value = matcher.group(4);

            if(key == null){
                key = matcher.group(7);
                value = matcher.group(8);
            }
            System.out.println(key + " => " + value);
            List<String>contentsList = parsedMap.get(key);
            if(contentsList == null) {
                contentsList = new ArrayList<String>();
                parsedMap.put(key, contentsList);
            }
            contentsList.add(value);
        }
        System.out.println(parsedMap);
        System.out.println(parsedMap.values().size());
    }
}

With the help of https://regex101.com I wrote the regex.

Improve this answer
\$\endgroup\$

Your Answer

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

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