2
\$\begingroup\$

I wrote this code to delete comments from an input string (such as a Java file converted to string) and it works great. For simple curiosity, I was wondering if there was a more elegant way of presenting the function that I wrote.

I'm looking for some advice on presenting the code, and how to condense the code.

Here's the function:

public static String deleteComments(String myString) {
    String newString = "";

    if (myString.contains("\"")) {
        if (myString.indexOf("\"") != 0) {

            String[] stringParts = myString.split("\"");

            for (int i = 0; i < stringParts.length; i++) {

                if ((i & 1) == 0) {
                    Pattern commentaryPattern = Pattern.compile("(/\\*((.|\n)*?)\\*/)|//.*");

                    Matcher m = commentaryPattern.matcher(stringParts[i]);

                    newString += m.replaceAll("");
                } else {
                    newString += "\"" + stringParts[i] + "\"";
                }
            }
        }
    } else {
        Pattern commentaryPattern = Pattern.compile("(/\\*((.|\n)*?)\\*/)|//.*");

        Matcher m = commentaryPattern.matcher(myString);

        newString += m.replaceAll("");
    }


    return newString;
}

Note: the regex pattern comes from another Stack Overflow link. It does most of the work but also deletes comments inside a string (eg: String myString = "bob said, \"Hi //To his friendly neighbor\".";) which, according to the java doc, is not a comment.

\$\endgroup\$
  • \$\begingroup\$ In theory, you should also check for \uxxxx character encodings. E.g. a quotation mark " can also be written as \u0022, and this can start and end a string literal. The same applies for all characters. But in real life, the risk introduced by ignoring that possibility is close to zero. \$\endgroup\$ – Ralf Kleberhoff Oct 18 '17 at 19:16
  • \$\begingroup\$ One more comment on the regex pattern. As it's constant (and Pattern.compile() is a complex and time-consuming task), you could move that out of your method and make it a static final constant (the Pattern, not the Matcher). \$\endgroup\$ – Ralf Kleberhoff Oct 18 '17 at 19:20
  • \$\begingroup\$ Seems like a good idea to move it out of the method! Thank you for pointing that out, I had previously moved it at the top of the method as Pattern commentaryPattern is repeated twice. \$\endgroup\$ – Elliott Addi Oct 19 '17 at 18:46
  • \$\begingroup\$ I just don't understand what can go wrong if writing \u0022 corresponds to " in unicode \$\endgroup\$ – Elliott Addi Oct 19 '17 at 18:49
  • \$\begingroup\$ The nasty thing about the \u0022is that it's ONLY translated by the java compiler, and not by java streams that you open in a java program. So, your program sees 6 characters instead of the single translated character. \$\endgroup\$ – Ralf Kleberhoff Oct 20 '17 at 15:51
0
\$\begingroup\$

Advice 1

newString += m.replaceAll("");

This is inefficient. Use StringBuilder for literally building the strings. Your approach works, but if you += \$n\$ characters one at a time, the time complexity is \$\Theta(n^2)\$. With StringBuilder it would be \$\Theta(n)\$.

Alternative implementation

The following is by no means a suggested implementation, but rather a starting point:

import java.util.Scanner;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class Main {

    public static String deleteComments2(String str) {
        StringBuilder sb = new StringBuilder(str.length());
        boolean inText = false;
        char previousCharacter = '\u0000';

        for (int i = 0; i < str.length(); ++i) {
            char currentCharacter = str.charAt(i);

            if (inText) {
                if (currentCharacter == '\"' && previousCharacter != '\\') {
                    inText = false;
                }
            } else {
                if (currentCharacter == '\"') {
                    if (previousCharacter == '\\') {
                        throw new RuntimeException("bad syntax");
                    }

                    inText = true;
                }

                if (currentCharacter == '/' && previousCharacter == '/') {
                    sb.deleteCharAt(sb.length() - 1);
                    break;
                }
            }

            sb.append(currentCharacter);
            previousCharacter = currentCharacter;
        }

        return sb.toString();
    }

    public static void main(String[] args) {
        Scanner scanner = new Scanner(System.in);

        while (true) {
            String str = scanner.nextLine();

            if (str.equals("quit")) {
                break;
            }

            System.out.println("1> " + deleteComments(str));
            System.out.println("2> " + deleteComments2(str));
            System.out.println("---");
        }
    }

    public static String deleteComments(String myString) {
        String newString = "";

        if (myString.contains("\"")) {
            if (myString.indexOf("\"") != 0) {

                String[] stringParts = myString.split("\"");

                for (int i = 0; i < stringParts.length; i++) {

                    if ((i & 1) == 0) {
                        Pattern commentaryPattern = Pattern.compile("(/\\*((.|\n)*?)\\*/)|//.*");

                        Matcher m = commentaryPattern.matcher(stringParts[i]);

                        newString += m.replaceAll("");
                    } else {
                        newString += "\"" + stringParts[i] + "\"";
                    }
                }
            }
        } else {
            Pattern commentaryPattern = Pattern.compile("(/\\*((.|\n)*?)\\*/)|//.*");

            Matcher m = commentaryPattern.matcher(myString);

            newString += m.replaceAll("");
        }

        return newString;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for pointing out the use of StringBuilder. I had never understood its use until now. I realised many people use a boolean to determine the state e.g boolean inText = false;: is it just to make code more easy to read or is it really more efficient as well? \$\endgroup\$ – Elliott Addi Oct 19 '17 at 18:57
  • \$\begingroup\$ @ElliottAddi I need inText in order to know that I am in a string and should not remove // occurring in it. \$\endgroup\$ – coderodde Oct 19 '17 at 19:05

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.