1
\$\begingroup\$

I have a text file with lines that look like this:

Robot(479.30432416307934|98.90610653676828)
Robot(186.42081184420528|213.11277688981409)
Robot(86.80794277768825|412.1359734884495)

or, more general:

Robot(DOUBLE|DOUBLE)

How should I parse it in Java?

I have written the following (tested and working) code:

private static List<Position> getPositions(File myFile)
        throws FileNotFoundException, IOException {
    List<Position> result = new ArrayList();
    BufferedReader br = new BufferedReader(new FileReader(myFile));
    try {
        String line = br.readLine();
        while (line != null) {
            String coordinates = line.substring(6, line.length() - 1);
            int splitpoint = coordinates.indexOf('|');
            double x = Double.parseDouble(coordinates.substring(0,
                    splitpoint));
            double y = Double.parseDouble(coordinates
                    .substring(splitpoint + 1));
            result.add(new Position(x, y));
            line = br.readLine();
        }
    } finally {
        br.close();
    }
    return result;
}

It looks a too complicated for such a simple task for me. I mean, with the RegEx

Robot\(\d+\.\d+|\d+\) 

I can match a line. According to Regextester my regex is correct. But how can I get the interesting part (the two doubles) from the regex?

Result after answer

/**
 * Read a file with positions of roboters.
 * @param file the file with positions
 * @return the list with roboter positions
 * @throws FileNotFoundException
 * @throws IOException
 */
private static List<Position> getPositions(final File file)
        throws FileNotFoundException, IOException {
    if (file == null || !file.canRead()) {
        throw new IllegalArgumentException("file not readable: " + file);
    }

    final Scanner s = new Scanner(file).useDelimiter("Robot\\(|\\||\\)\n?");
    final List<Position> positions = new ArrayList<Position>();
    while (s.hasNext()) {
        positions.add(new Position(s.nextDouble(), s.nextDouble()));
        s.nextLine();
    }

    return positions;
}
\$\endgroup\$
1
\$\begingroup\$

java.util.Scanner provides a nice way to split a given string by regex:

import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;


class Robot {

  final double d1;
  final double d2;

  public Robot(final double d1, final double d2) {
    this.d1 = d1;
    this.d2 = d2;
  }

  @Override
  public String toString() {
    return "Robot(" + d1 + "|" + d2 + ")";
  }
}

public class Test {

  public static void main(final String[] args) {
    final String str =
          "Robot(479.30432416307934|98.90610653676828)\n"
        + "Robot(186.42081184420528|213.11277688981409)\n"
        + "Robot(86.80794277768825|412.1359734884495)";
    final Scanner s = new Scanner(str).useDelimiter("Robot\\(|\\||\\)\n?");
    final List<Robot> robots = new ArrayList<Robot>();
    while (s.hasNext()) {
      robots.add(new Robot(s.nextDouble(), s.nextDouble()));
      s.nextLine();
    }
    System.out.println(robots);
  }
}

The only flaw is that after each line an empty string is read. I couldn't find out why - thus, I discarded it manually.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ The flaw is not easily visible in your representation. If you write he string in one line, it is easier to see. If we look at this part: 28)\nRobot(186 and think about your regexp, we will see, that \)\n? happily matches )\n. But then, the Scanner is at position after this. Before Robot(186. If you want to avoid this, you have to catch the following Robot(, too. One easy way would be to catch all non-digits, because the next interesting char is a digit: Robot\\(|\\||\\)\\D*. But I would not suggest to use this regexp, this is already hardly unreadable. \$\endgroup\$ – tb- Mar 1 '13 at 21:19
  • \$\begingroup\$ @tb: Thanks for the explanation, I see the problem now. I agree, the regex is difficult to read and the single s.nextLine() isn't that bad to obfuscate it further... \$\endgroup\$ – kiritsuku Mar 1 '13 at 21:23
2
\$\begingroup\$

As this is posted on codereview, let us take a look at the whole function.

private static List<Position> getPositions(File myFile)
        throws FileNotFoundException, IOException {

I would suggest to do a check for the file (and rename it to file, I do not think there is a herFile, hisFile and so on):

    if (file == null || !file.canRead())
        throw new IllegalArgumentException("file not readable: " + file);

Which could make error handling a bit easier. We will see later.


    List<Position> result = new ArrayList();
    BufferedReader br = new BufferedReader(new FileReader(myFile));
    try {
        String line = br.readLine();
        while (line != null) {
        ...
            line = br.readLine();

I would keep the typical pattern, so everyone can recognize it:

        String line;
        while ((line = bufferedReader.readLine()) != null) {

            String coordinates = line.substring(6, line.length() - 1);
            int splitpoint = coordinates.indexOf('|');
            double x = Double.parseDouble(coordinates.substring(0, splitpoint));
            double y = Double.parseDouble(coordinates.substring(splitpoint + 1));
            result.add(new Position(x, y));
            line = br.readLine();
        }

For me, this is nearly fine. Just to a split("|") and you are done in the easy way:

            final String[] coordinatesArray = line.substring(6, line.length() - 1).split("|");
            final double x = Double.parseDouble(coordinatesArray[0]);
            final double y = Double.parseDouble(coordinatesArray[1]);
            result.add(new Position(x, y));

And I suggest to add an example in a comment, to help the next reader.


    } finally {
        br.close();
    }

If you have Java7, you could change this to try-with statement:

    try (final BufferedReader bufferedReader = new BufferedReader(new FileReader(file))) {
        ...
    }        

And you could catch the error. The most problematic ones are cought by the file checks. Everything else is probably something you can not do anything about.
Then it is fine to catch it, show it and continue:

    } catch (NumberFormatException | IOException e) {
        e.printStackTrace(); // handle the exception or rethrow it. It could be safe to do nothing here
    }

If you put everything together, it could look like this:

private static List<Position> getPositions(final File file) {
    if (file == null || !file.canRead())
        throw new IllegalArgumentException("file not readable: " + file);

    final List<Position> result = new ArrayList<>();
    try (final BufferedReader bufferedReader = new BufferedReader(new FileReader(file))) {
        String line;
        while ((line = bufferedReader.readLine()) != null) {
            // example: Robot(479.30432416307934|98.90610653676828)
            final String[] coordinatesArray = line.substring(6, line.length() - 1).split("|");
            final double x = Double.parseDouble(coordinatesArray[0]);
            final double y = Double.parseDouble(coordinatesArray[1]);
            result.add(new Position(x, y));
        }
    } catch (NumberFormatException | IOException e) {
        e.printStackTrace(); // handle the exception or rethrow it. It could be safe to do nothing here
    }
    return result;
}

If you try the way with the scanner, you shoudl just tell him which characters to ignore. Please do not try a complex regular expression. They are unreadable.
What we have there?

Robot(479.30432416307934|98.90610653676828)

Well, everything which is not a number or a point. To save some typing, we do not place every character alone, we put them together as \p{Alpha} (any alphabetic character): [\p{Alpha}()|\]. And we want this to happen for one or more times: [\p{Alpha}()|]+. And we should not forget about multiple lines, then there is a newline: [\p{Alpha}()|\\n]+.

It is still mostly readable if you have a basic understanding of regular expressions, so we are fine in this case. You could also use [a-zA-Z()|\\n]+ which would be more common outside the java world.

The complete function could look like this:

private static List<Position> getPositions(final File file) {
    if (file == null || !file.canRead())
        throw new IllegalArgumentException("file not readable: " + file);

    final List<Position> result = new ArrayList<>();

    try (Scanner scanner = new Scanner(file)) {
        // example: Robot(479.30432416307934|98.90610653676828)
        scanner.useDelimiter("[\p{Alpha}()|\\n]+");
        while (scanner.hasNext())
            result.add(new Position(scanner.nextDouble(), scanner.nextDouble()));
    } catch (final FileNotFoundException e1) {
        e1.printStackTrace(); // handle the exception or rethrow it. It could be safe to do nothing here
    }
    return result;
}
\$\endgroup\$
0
0
\$\begingroup\$

I would suggest the use of parethensis to store these values:

String regexp = "Robot\((\d+\.\d+)|(\d+\.\d+))";

Pattern pattern = Pattern.compile(regexp);
Matcher m1 = pattern.match(line);

while (m1.find()) {
    final int count = m1.groupCount();
    // group 0 is the whole pattern matched,
    // loops runs from from 0 to gc, not 0 to gc-1 as is traditional.
    for ( int i = 0; i <= gc; i++ ) {
        out.println( i + " : " + m1.group( i ) );
    }
}

Source: http://www.mindprod.com/jgloss/regex.html #Finding

\$\endgroup\$
2
  • \$\begingroup\$ Please take it as a hint, I haven't had the time to test it thoroughly right now :) \$\endgroup\$ – David M. Mar 1 '13 at 18:51
  • \$\begingroup\$ This does not work this way. Even if we fix the small typos, the regexp is declared as a group with the numbers inside. This wont work and will catch at most the last number. It could be fixed with some work, but I do not think this makes it easier to understand or to apply. \$\endgroup\$ – tb- Mar 1 '13 at 20:37

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