2
\$\begingroup\$

This checks whether it's valid to make a processing. Processing can occur if:

  1. there are files and spreadsheets (spreadsheet is the spec attribute)
  2. there are any files or spreadsheets (it never occurs if there is at least one of the two)

AllFiles is empty only if one has sent a zip, but it wasn't unzipped and has been deleted. If unzipped and deleted, allFilesIsEmpty is false because one usually receives files in addition to the formats allowed that the program doesn't delete.

It should be at least one .swf, .gif or .jpg processing to occur. If not, it's not an allowed file format.

I described the best in the code but I have cyclomatic complexity. Any idea on how I can reduce?

private boolean directoryIsEmpty(OldValidacao validacao) {
        Map<String, String> allFiles = validacaoService.getFilenameMap(validacao.getArquivoPath());
        if (allFiles.isEmpty() && validacao.getSpec() == null) {
            return true;
        } else if (validacao.getSpec() != null) {
            return false;
        } else if (!allFiles.isEmpty()) {
            String files = allFiles.toString();
            if (files.contains("swf") || files.contains("jpg") || files.contains("gif")) {
                return false;
            }
        }
        return true;
    }
\$\endgroup\$
  • \$\begingroup\$ One VERY annoying thing is that the variable names are in Portuguese. You should write the names in English, or your code is only usable for Brazil and Portugal. The mix of English and Portuguese really ticks a lot, and it makes hard (for non-Portuguese readers) to understand what for is that variable. Code should be self-explainatory. \$\endgroup\$ – Ismael Miguel Jun 16 '15 at 17:35
  • \$\begingroup\$ There is no convention on the team as the language, what causes this english with portuguese. Thanks for the tips \$\endgroup\$ – Daniela Morais Jun 16 '15 at 17:37
  • 2
    \$\begingroup\$ The convention should be to write code exclusively in English. I'm Portuguese and I understand it. Maybe I'm the only Portuguese here. And I don't program in Java. How will we help you in this situation? (I'm not saying that the question doesn't have quality. It has! I'm saying that the linguistic barrier won't be broken if we write code that has more than 1 spoken language.) \$\endgroup\$ – Ismael Miguel Jun 16 '15 at 17:40
3
\$\begingroup\$

It should be at least one .swf, .gif or .jpg processing to occur. If not, it's not an allowed file format.

The way try to check this is not so good:

    String files = allFiles.toString();
    if (files.contains("swf") || files.contains("jpg") || files.contains("gif")) {
        return false;
    }

You should not rely on the .toString() implementation to verify values. You should use the values. I suggest to rewrite like this or similar:

for (String filename : allFiles.values()) {
    if (filename.endsWith(".swf") || filename.endsWith(".jpg") || filename.endsWith("gif")) {
        return false;
    }
}

This loop iterates over the values, and I replaced the .contains check with .endsWith, which seems to be more accurately matching what you really want to check. Another alternative to write the condition:

if (filename.matches(".*(\\.swf|\\.jpg)$")) {
    return false;
}

I described the best in the code but I have cyclomatic complexity. Any idea on how I can reduce?

It seems you want to reduce cyclomatic complexity of the method. The main contributors to the high cyclomatic complexity in your example:

  • many return statements
  • many if conditions
  • many && and || operators

One way to reduce cyclomatic complexity is to extract some parts of the method to another method, for example:

if (allFiles.isEmpty() && validacao.getSpec() == null) {
    return true;
} else if (validacao.getSpec() != null) {
    return false;
} else if (!allFiles.isEmpty()) {
    if (containsMediaFiles(allFiles)) {
        return false;
    }
}
return true;

I just invented the helper method containsMediaFiles, and eliminated a for loop and some || conditions.

But in this particular example, there's an even easier way. Realize that you don't really need to check if allFiles is not empty. You can just loop over it, and if it's empty, there will be nothing to loop over, so the empty case will be automatically handled, like this:

private boolean directoryIsEmpty(OldValidacao validacao) {
    Map<String, String> allFiles = validacaoService.getFilenameMap(validacao.getArquivoPath());

    if (allFiles.isEmpty() && validacao.getSpec() == null) {
        return true;
    }

    if (validacao.getSpec() != null) {
        return false;
    }

    for (String filename : allFiles.values()) {
        if (filename.matches(".*\\.(swf|jpg|gif)$")) {
            return false;
        }
    }

    return true;
}

@ismael-miguel made a good point in comments that probably you want to match extensions like "JPG", and even "JPg". An easy way to do that is to convert filename to lowercase before the .matches call above. And you might want to add more patterns, for example jpeg, or use the pattern jpe?g to take care of both "jpeg" and "jpg" at the same time.

\$\endgroup\$
  • \$\begingroup\$ The only thing I would change is the regex to ".*(\\.swf|\\.jpe?g|\\.gif)$", and making it case-insensitive. \$\endgroup\$ – Ismael Miguel Jun 17 '15 at 1:03
  • \$\begingroup\$ You're welcome. It's worth noting that jpg and jpeg file are the same, except that jpg is used for filenames in the format 8.3. \$\endgroup\$ – Ismael Miguel Jun 17 '15 at 8:56
  • \$\begingroup\$ I liked your answer and found elegant code, I added the application. But only after I found the bug: It may be the map that the first element is an illegal file (format other than swf , gif or jpg) and the second element is an allowed format. This doesn't let you do the processing when it is valid. \$\endgroup\$ – Daniela Morais Jun 18 '15 at 11:38
  • \$\begingroup\$ It's a bit difficult to understand what the bug is. My proposed solution is equivalent to your original program, just slightly improved. If the program doesn't behave exactly as you want, and you don't know how to fix it, then it's better to ask on Stack Overflow, including example input, and example output in contrast with your desired output. Feel free to post a link to it from here, I'll be happy to look at it and maybe answer there. In the context of this current question, I simply don't have enough information to help you. \$\endgroup\$ – janos Jun 18 '15 at 12:18
2
\$\begingroup\$

As my first point, and already mentioned in a comment, all variable names should be written in English.
This makes it possible for non-Portuguese speakers to understand the code.
The code should be self-explainatory.


The method name should be isValidDirectory.
Boolean methods begin with isSomething, like if you were asking a question.
Since you are 'asking' if a directory has some defined attributes, you should reflect this in your method name.

Based on your current name, I wouldn't expect it to return true if it had a file there.

One example of this is the method String.isNullOrEmpty.

You have another example on the following line:

if (allFiles.isEmpty() && validacao.getSpec() == null) {

Taken from your code.


At the end, you have the following code:

    } else if (!allFiles.isEmpty()) {
        String files = allFiles.toString();
        if (files.contains("swf") || files.contains("jpg") || files.contains("gif")) {
            return false;
        }
    }

This can be changed into this (assuming the change in the method name):

    } else if (!allFiles.isEmpty()) {
        String files = allFiles.toString();
        return files.contains(".swf") && files.contains(".jpg") && files.contains(".gif");
    }

Since that expression will return a boolean value, you can send it directly.
Also, I've added a . to the filenames, because one could name a file jpg.c and it would validate.

I don't think this is the right way to check if those parameters are met, since you may have a file named a.jpeg and it would fail, even though it is a perfectly valid extension.
Even a.JPG would fail!

Java isn't my beach and it really is outside my confort area, but I recommend a method where you itterate over all the elements and check, with a Regular Expression, if the extension is indeed valid.

\$\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.