3
\$\begingroup\$

I have about 1000 .xml files in different sub directories under one root folder.

My root directory contains the below structure:

  • RootDir\172\00ad8dc-bff\25.0.xml
  • RootDir- The root directory for all my .xml files.
  • 172- The sub directory of the "RootDir" directory. There are many such directories (I gave 172 as an example)
  • 00ad8dc-bff- The sub directory under "172" directory. There are many such directories.
  • 25.0.xml- At this level I have my target .xml files in which the search should be performed.

Conditions on the search:

My program should not search if,

  1. There is a folder with name "processes".
  2. The file name is "draft.xml".

There may be many files under this folder (00ad8dc-bff), each representing different version of the .xml file. In this case the version of the .xml file is 25 (obviously this would be the last modified file), search only in the latest version of the .xml file (last modified file). (Note: .xml file names indicate the version of the .xml file).

To achieve the above use case I wrote the below program. The below .xml is "25.xml" so my program should return "my_uuid" and "My Name goes here" if I give for example "4458" as the search string.(4458 text is present in the given .xml file).

My .xml file looks like below and my program is provided right below this .xml file.

   <?xml version="1.0" encoding="UTF-8"?>
 <my_port
xmlns="http://www.mytest.com/test"
xmlns:a="http://www.mytest.com/"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xmlns:xsd="http://www.w3.org/2001/XMLSchema" schemaVersion="007.000.001">
<second-node>
    <meta>
        <uuid>
            <![CDATA[my_uuid]]>
        </uuid>
        <name>
            <string-map>
                <pair>
                    <locale lang="en" country="GB" variant="" />
                    <value>
                        <![CDATA[My Name goes here]]>
                    </value>
                </pair>
            </string-map>
        </name>
        <desc>
            <string-map>
                <pair>
                    <locale lang="en" country="GB" variant="" />
                    <value>
                        <![CDATA[My description]]>
                    </value>
                </pair>
            </string-map>
        </desc>
        <version>
            <![CDATA[56.0]]>
        </version>
        <versionStatus>2</versionStatus>
        <latestVersion>
            <![CDATA[56.0]]>
        </latestVersion>
        <state id="12"/>
        <pr-name>
            <string-map>
                <pair>
                    <locale lang="en" country="GB" variant="" />
                    <value>
                        <![CDATA[ppname]]>
                    </value>
                </pair>
            </string-map>
        </pr-name>
        <deadline>
            <enabled>false</enabled>
            <type>0</type>
            <units>0</units>
            <rex>
                <![CDATA[]]>
            </rex>
            <aex>
                <![CDATA[]]>
            </aex>
        </deadline>
        <pm-notification-settings>
            <custom-settings>true</custom-settings>
            <notify-initiator>
         false</notify-initiator>
            <notify>false</notify>
            <usrsandgrps>
                <usr>
                    <type>4584</type>
                    <stringId>
                        <![CDATA[]]>
                    </stringId>
                </usr>
                <usr>
                    <type>4458</type>
                    <stringId>
                        <![CDATA[]]>
                    </stringId>
                </usr>
            </usrsandgrps>
            <recipients-exp/>
        </pm-notification-settings>
        <cleanup>3</cleanup>
        <arch>30</arch>
        <delete>0</delete>
        <timeZoneId>
            <![CDATA[Country]]>
        </timeZoneId>
        <dummyTZ>true</dummyTZ>
    </meta>
    <vrprs>
        <vrpr name="name1">
            <a:value
                xmlns="" xsi:type="dummy" />
                <para>true</para>
                <req>false</req>
                <hide>false</hide>
            </vrpr>
            <vrpr name="name440">
                <a:value
                    xmlns="" xsi:type="dummy" />
                    <para>false</para>
                    <req>false</req>
                    <hide>false</hide>
                </vrpr>
                <vrpr name="name75">
                    <a:value
                        xmlns="" xsi:type="dummy">0
                    </a:value>
                    <para>true</para>
                    <req>true</req>
                    <hide>false</hide>
                </vrpr>
                <vrpr name="name66">
                    <a:value
                        xmlns="" xsi:type="dummy">0
                    </a:value>
                    <expr>
                        <![CDATA[=0]]>
                    </expr>
                    <para>false</para>
                    <req>false</req>
                    <hide>false</hide>
                </vrpr>
                <vrpr name="name44">
                    <a:value
                        xmlns="" xsi:type="dummy" />
                        <para>false</para>
                        <req>false</req>
                        <hide>false</hide>
                    </vrpr>
                    <vrpr name="name55">
                        <a:value
                            xmlns="" xsi:type="dummy" />
                            <para>true</para>
                            <req>false</req>
                            <hide>false</hide>
                        </vrpr>
                        <vrpr name="name4">
                            <a:value
                                xmlns="" xsi:type="xsd:int">0
                            </a:value>
                            <expr>
                                <![CDATA[=0]]>
                            </expr>
                            <para>false</para>
                            <req>false</req>
                            <hide>false</hide>
                        </vrpr>
                    </vrprs>
                    <nodes>
                        <node uuid="mYnodeUUID">
                            <guiId>0</guiId>
                            <owner>
                                <![CDATA[]]>
                            </owner>
                            <icon id="50"></icon>
                            <picon id="0"></picon>
                            <fname>
                                <string-map>
                                    <pair>
                                        <value>
                                            <![CDATA[Name of Nd]]>
                                        </value>
                                    </pair>
                                </string-map>
                            </fname>
                            <x>28</x>
                            <y>280</y>
                            <display>
                                <str-map>
                                    <pair>
                                        <locale lang="en" country="GB" variant="" />
                                        <value>
                                            <![CDATA[Name ofdd]]>
                                        </value>
                                    </pair>
                                </str-map>
                            </display>
                            <desc>
                                <str-map>
                                    <pair>
                                        <value>
                                            <![CDATA[]]>
                                        </value>
                                    </pair>
                                </str-map>
                            </desc>
                            <notify>false</notify>
                            <ac>
                                <local-id>core.0</local-id>
                                <name>
                                    <![CDATA[Name ofdd]]>
                                </name>
                                <ni-default-security>
                                    <can-inherit>true</can-inherit>
                                    <does-inherit>true</does-inherit>
                                    <native-role-map></native-role-map>
                                </inherited-role-map>
                            </ni-default-security>
                            <lingering>false</lingering>
                            <on-create-ignore-if-active>false</on-create-ignore-if-active>
                            <on-create-delete-previous-active>false</on-create-delete-previous-active>
                            <on-complete-delete-previous-completed>false</on-complete-delete-previous-completed>
                            <pre-triggers></pre-triggers>
                            <post-triggers></post-triggers>
                            <event-producers></event-producers>
                            <exception-flow></exception-flow>
                            <label>
                                <fontColor>#000000</fontColor>
                                <fontFamily>Arial, Helvetica, Sans-Serif</fontFamily>
                                <fontSize>12</fontSize>
                                <bold>false</bold>
                                <italics>false</italics>
                                <underline>false</underline>
                            </label>
                            <deadline>
                                <enabled>false</enabled>
                                <type>0</type>
                                <units>0</units>
                                <rex>
                                    <![CDATA[]]>
                                </rex>
                                <aex>
                                    <![CDATA[]]>
                                </aex>
                            </deadline>
                            <allowsBack>false</allowsBack>
                            <refreshDefaultValues>false</refreshDefaultValues>
                            <on-complete-keep-form-data>false</on-complete-keep-form-data>
                        </node>
                    </nodes>
                    <annotations></annotations>
                    <lanes/>
                    <attachments></attachments>
                    <notes>
                    </second-node>
                </my_port>

I am using code as below to search through all .xml files, but I am not sure it is the best way:

import java.io.BufferedReader;
import java.io.File;
import java.io.FileReader;
import java.io.IOException;
import java.util.ArrayList;

public class PMXmlUtil {
    static BufferedReader br = null;
    static ArrayList<String> uuids = new ArrayList<String>();
    //The below is the text that I am searching for
    static String searchQuery = "4458";
    public static String rootDirectory = null;

    public static void main(String[] args) {
        rootDirectory = getSearchLocation();
        int totalXMLFiles = 0;
        if (rootDirectory != null && rootDirectory.length() > 0) {
            File f = new File(rootDirectory);
            try {
                if (f.isDirectory()) {
                    File[] subDir = f.listFiles();
                    for (File fi : subDir) {
                        File[] xmlRootDirs = fi.listFiles();
                        for (File file : xmlRootDirs) {
                            if (file.isDirectory()) {
                                File xmlFiles[] = file.listFiles();
                                File lastModifiedFile = xmlFiles[0];
                                for (int i = 1; i < xmlFiles.length; i++) {
                                    if (lastModifiedFile.lastModified() < xmlFiles[i].lastModified()) {
                                        lastModifiedFile = xmlFiles[i];
                                    }
                                }
                                try {
                                    if (!lastModifiedFile.getName().contains("processes")) {
                                        if (!lastModifiedFile.getName().contains("draft")) {
                                            br = new BufferedReader(new FileReader(lastModifiedFile));
                                            search();
                                        }
                                    }
                                } catch (Exception e) {
                                    e.printStackTrace();
                                } finally {
                                    if (br != null) {
                                        try {
                                            br.close();
                                        } catch (IOException e) {
                                            e.printStackTrace();
                                        }
                                    }
                                }
                                totalXMLFiles++;
                            }
                        }
                    }
                }
            } catch (Exception e) {
                e.printStackTrace();
            }
        }
        System.out.println(uuids);
    }

    private static String getSearchLocation() {
        return "C:\\myname\\Desktop\\myRootFolder";
    }

    private static void search() throws IOException {
        boolean uuidLineRead = false;
        String tempUUIDLine = null;
        String currentLine;
        while ((currentLine = br.readLine()) != null) {

            String tempTargetTextLine = null;
            if (currentLine.contains("uuid") && !uuidLineRead) {
                tempUUIDLine = currentLine;
                uuidLineRead = true;
            }
            if (currentLine.contains(searchQuery)) {
                tempTargetTextLine = currentLine;
                uuids.add(tempUUIDLine);
                break;
            }
        }
    }
}
\$\endgroup\$
8
  • \$\begingroup\$ Do you actually want to parse your XML files into a Document before searching through that, or is this really more of a text-based search? If it is the latter, I will suggest removing the xml tag. If it is the former, then I actually think this will be a more interesting question. \$\endgroup\$
    – h.j.k.
    Jan 13, 2015 at 5:46
  • \$\begingroup\$ hi @h.j.k. this really more of a text-based search. I don't want to parse XML files into a Document before searching through. \$\endgroup\$ Jan 13, 2015 at 5:48
  • 1
    \$\begingroup\$ Ok... also, will you be able to use Java 8's new APIs for traversing directory structures, or are you coding strictly on pre-Java 8? \$\endgroup\$
    – h.j.k.
    Jan 13, 2015 at 5:50
  • 1
    \$\begingroup\$ It is pre java 8. All my clients and servers are not upgraded to java 8 yet. we are on Java 7. \$\endgroup\$ Jan 13, 2015 at 5:51
  • 1
    \$\begingroup\$ Also, it's fine to clarify and elaborate the non-code parts your question, but please keep code changes to a minimum, e.g. correcting simple typos. \$\endgroup\$
    – h.j.k.
    Jan 14, 2015 at 5:51

3 Answers 3

3
\$\begingroup\$

The main problem with your code is that you are doing far too much before the actual text search. This is what I can summarize from your code:

  1. Check if given path A is a directory.
  2. If path A is a directory, we attempt to get an array of paths A/B* inside A.
  3. For each of the path A/B in A/B*, we attempt to get an array of paths A/B/C* inside each A/B.
  4. For each of the path A/B/C in A/B/C*, we check if that is a directory.
  5. If A/B/C is a directory, we attempt to get an array of paths A/B/C/D* and then we determine the latest modified file A/B/C/D. If there are multiple latest files, then by default the first iterated is our 'target file'.
  6. If A/B/C/D does not contain processes or draft in its name, then we create a BufferedReader for it, referenced by a static variable, and then proceed with search().
    • Search results are saved in a static variable uuids.
  7. If A/B/C/D contains processes or draft in its name, we skip processing in the current A/B/C directory and go to the next one.

There are a few critical issues from the above:

  • Nested indentation runs deep, because every step is nested in the preceding if or for statement.
    • Even if you want to clump the logic together, you should try to check for terminal conditions first. For example, If step 1 above is not true, then your program can pretty much exit. Steps 2 to 7 need not be nested inside the if statement in step 1.
    • Also, you may have only up to two levels of directories, but what happens when you have more in the future?
  • static variables are used over properly designed method signatures, which may will get confusing once you have more 'arguments' to deal with throughout your code.
  • Do you really want to ignore \$n\$ files when \$n + 1\$ have the same latest modified timestamps in step 5?
  • I am not sure if your logic for step 7 is right: We think we found the latest file, but if the file name is not the one we want, we go to the next sibling directory (i.e. A/B/C1/ after A/B/C2/)? Do we even want to check that maybe the second latest file (see above issue on point 5) should be searched as well?

My suggestion is quite simple: take a step back and think about what your code needs to do:

  • A method returning a Set<File> that needs to be searched, it can be something like:

    public static Set<File> getFileCandidates(final String rootDirectory);
    
  • A method that takes in a File, access that file, and returns matching lines:

    // assuming the seach term is fixed
    public static List<String> getMatchingLines(final File file); 
    

And that's about it:

public static void main(String[] args) {
    String rootDirectory = args[0]; // for example
    final List<String> results = new ArrayList<>();
    for (final File fileCandidate : getFileCandidates(rootDirectory)) {
        results.addAll(getMatchingLines(fileCandidate));
    }
    System.out.println(results);
}

Where appropriate, you should have tighter try-catch blocks so that any I/O errors (in your case) can be handled individually, without affecting the flow of your code.

edit #1 (2015-01-14):

Your added details safely eliminates some of the potential issues I described (e.g. number of nested directories and last modification timestamps), but my suggestion still applies.

However, your (slightly-edited) code now does not do what you have described. This is what your code is doing:

  1. If current line is the first to contain "uuid", 'save' this line (tempUUIDLine) and continue to process other lines not containing "uuid".
  2. If the current line contains searchQuery, print tempUUIDLine.

Based on your sample content for 25.0.xml, the first matching line is <uuid>, so I don't see how it will

return "my_uuid" and "My Name goes here" if I give for example "4458" as the search string.

This highlights a couple more things:

  • Your code is broken, even though you may think it is working to the best of your knowledge. If this is a case, do consider posting a SO question to fix how your search() is being done first, before posting a follow-up review here.
  • If you end up having to scan through multiple lines forwards/backwards within the file to do your data extraction, you really might be better off parsing the XML file into a Document after all to get your results using a more standardized solution.

As for the directory traversal, what I can briefly suggest is to use a combination of Files.newDirectoryStream and DirectoryStream.Filter to help you filter directories and files. Your current way of checking:

!lastModifiedFile.getName().contains("processes") && 
    !lastModifiedFile.getName().contains("draft")

Will fail for paths like ...\draft.txt or ...\processes.pdf. Of course, if you know for sure that you will not have these cases...


Java: Be all, end all?

Do you really need a Java application for this? If your target OS is *nix, you can pretty much achieve the same thing with shell scripts, using a combination of find and grep for example. And that's off the top of my head...

\$\endgroup\$
1
\$\begingroup\$

An option to consider

Delegate the file processing /selection to something like gradle, Ant, Maven - leaving you with a single java (or Groovy?) class that does just a check per file of the text you need and perhaps appends results to a final location / file / sysout?

Input arguments: XML File location, Output file location?

It's not clear from your sample if you are searching for known XPath locations or just general text at any location in the XML:

If it's 2 known XPath locations

/myport/second/meta/uuid : value() /myport/second/stringmap/pair/value : value()

Consider the options of:

Parsing? * SAX (Fastest parsing, more complex code - event handling) * StAX (Compromise speed vs. implementation complexity) * DOM ( Higher memory footprint, simpler to code)

Groovy / XML Parsing - https://today.java.net/pub/a/today/2004/08/12/groovyxml.html

Look at the 'Wrapping the XML DOM' sample in the above article as a sample of using these XPath arguments.

This should reduce the amount of code you have to write and separate the file matching from the XML parsing, value selection.

\$\endgroup\$
1
\$\begingroup\$

Instead of the ugly try-catch-finally in the main method, setting the br static variable to be processed by search, it would be better to make search take a File parameter and create a BufferedReader locally.

The tempTargetTextLine in search is unused, so you should remove it.

The checks rootDirectory != null && rootDirectory.length() > 0 in main are really absurd, as rootDirectory is a hard-coded string in your code.

The totalXMLFiles variable is assigned and incremented, but never used.

File.listFiles() may return null, for example if you don't have permission to access the contents of the directory, so you should add a check for nulls every time you use this.

Wrapping a large block of code within a try-catch like you did in main is a terrible idea. You should limit the scope of try blocks to as minimal as possible.

The indentation was originally a complete mess in your code. I fixed it in your question itself for everybody's sake. In the future use your IDE to automatically format the code nicely.

\$\endgroup\$
0

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.