1
\$\begingroup\$

I have a function that breaks up a source file and checks each token against a keyword list to determine whether its a particular file type. How can I improve this function? Tokenization is a class that contains a function to break up the source code.

public static boolean validationTypeOfSQL(String sourceFile, Tokenization tokenization,ArrayList<String> uniqueKeywordList) {
    ArrayList<Token> createSourceCodeTokens = tokenization.createSourceCodeTokens(sourceFile);

    for (Token x : createSourceCodeTokens) {
        String smallerTokens[] = tokenization.tokenToSmallerToken(x.getToken());
        for (String smallerTOkenValue : smallerTokens) {
            Boolean result = smallerTOkenValue.contains(",");
            if (result == true) {
                String[] tokenSplit = smallerTOkenValue.split(",");
                for (String ts : tokenSplit) {
                    for (String t : uniqueKeywordList) {
                        if (t.toLowerCase().equals(ts.toLowerCase().replace("(", "").replace(",", "").replace(")", "").replace("\t", ""))) {
                            // System.out.println("This is a mysql file Mysql");
                            return true;
                        }
                    }
                }
            }
            else {
                for (String uniqueKeyword : uniqueKeywordList) {
                    if (uniqueKeyword.toLowerCase().equals(smallerTOkenValue.toLowerCase().replace("(", "").replace(",", "").replace(")", ""))) 
                    {
                        return true;
                    }
                }
            }
        }

    }
    return false;
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Do you have an example or unit test to illustrate what you're actually attempting to do here? \$\endgroup\$ – h.j.k. Jul 16 '15 at 8:01
  • \$\begingroup\$ When writing these kinds of validations I often consider "Which files do I not want to include?" and "What other files will I be running this validation against?" answering those two questions is quite important to find out what it is that you actually need. \$\endgroup\$ – Simon Forsberg Jul 16 '15 at 10:46
1
\$\begingroup\$

I find that it is not good practice to pass in a argument by its implemented name if it has a backing interface. ArrayList<T> is a good example, it is based on the interface List<T> and is just as capable as ArrayList. It is useful this way because if you decide to pass in an immutable List, or a different type of List you don't have to worry about fixing compiler errors down the road.

Having nested for loops and if statements makes it difficult to know where and what to do. I find it often easier to take the contents inside a for loop and put them in a extracted method giving it a good name.

This little bit of code is hard to read. plus comparing a boolean is not good practice.

Boolean result = smallerTOkenValue.contains(",");
if (result == true) {

consider replacing it with this if (smallerTOkenValue.contains(",")){

I'm going to assume that this was a mistake smallerTOkenValue.toLowerCase().replace("(", "").replace(",", "").replace(")", "") specifically .replace(",", "") because a few lines above you ask if (smallerTOkenValue.contains(",")) and if it did you split it up. Which means that your smallerTOkenValue should not contain any ','

Lastly I believe you can actually use a small form of recursion to simplify your code some (providing of course you split your big method into smaller methods). below is my final outcome. (I have not tested this, as I'm typing this in notepad++). Notice that the second to last method uses calls itself.

public static boolean validationTypeOfSQL(String sourceFile, Tokenization tokenization, List<String> uniqueKeywordList) {
    List<Token> createSourceCodeTokens = tokenization.createSourceCodeTokens(sourceFile);

    for (Token x : createSourceCodeTokens) {
        if(isSourceCodeTokenValid(x, uniqueKeyWord))
            return true;
    }
    return false;
}
private static boolean isSourceCodeTokenValid(Token x, List<String> uniqueKeywordList){
    String smallerTokens[] = tokenization.tokenToSmallerToken(x.getToken());
    for (String smallerTOkenValue : smallerTokens) {
        if(smallerTokenIsValid(smallerTOkenValue, uniqueKeyWord))
            return true;
    }
    return false;
}
private static boolean smallerTokenIsValid(String smallerTOkenValue, List<String> uniqueKeywordList){
    if (smallerTOkenValue.contains(",")) {
        for (String ts : smallerTOkenValue.split(",")) {
            smallerTokenIsValid(ts, uniqueKeywordList);
        }
    }
    else {
        for (String uniqueKeyWord : uniqueKeywordList) {
            if(uniqueKeywordIsEqualToTokenValue(uniqueKeyWord, smallerTOkenValue)){
                return true;
            }
        }
    }
    return false;
}
private static boolean uniqueKeywordListIsEqualToTokenValue(String uniqueKeyWord, string smallerTOkenValue){
    string strippedTokenValue = smallerTOkenValue.toLowerCase().replace("(", "").replace(")", "").replace("\t", "");
    return uniqueKeyWord.toLowerCase().equals(strippedTokenValue))
}
\$\endgroup\$
2
\$\begingroup\$

Regarding your naming:

I'd rename:

  1. validationTypeOfSQL() to isSQL()
  2. createSourceCodeTokens to sourceCodeTokens
  3. createSourceCodeTokens() to sourceCodeTokens()
  4. x to token
  5. tokenToSmallerToken() to smallerTokens()
  6. smallerTOkenValue to smallerToken
  7. result to containsComma (It's better, of course, to remove it completely according to Robert Snyder's answer.)
  8. tokenSplit to splitTokens
  9. ts to st or splitToken
  10. t to k or keyword
  11. uniqueKeyword to keyword

I'd change String smallerTokens[] to String[] .... as you write it 4 lines later anyway.

I' d use replaceAll(String regex, String replacement) with replaceAll("[(,)\t]", "") instead of the replace() chain.

\$\endgroup\$

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.