1
\$\begingroup\$

I have to check an uploaded file for anti-virus and if there is an exception, I have to give a message according to the exception to the users. e.g. if its a timeout, I would ask the user to try againg in a while.

My first class, fileUpload is:

public class FileUpload {

    //Message when no virus found.     
    private static final String NO_VIRUS_FOUND =  "No virus found";


    //More attributes and functions


    public void checkFile(FacesContext ctx, UIComponent comp, Object value) throws IOException {
          file = (Part) value;
          if (file != null) {
                  String antiVirusResult;    
                  antiVirusResult = FileCheckClass.checkVirus(getConfig(), file.getInputStream(), NO_VIRUS_FOUND);
                 if (!antiVirusResult.contains(NO_VIRUS_FOUND)) {

                     generateAntiVirusMessages(antiVirusResult);
                     throw new ValidatorException(msgs);
                 }
          }
     }

}

My seond class, FileCheck class has a checkVirus() function:

public static String checkVirus(FileConfig fileconfig, InputStream inputStream, String noVirusFound) {
        try {

            ClamAVClient cl = new ClamAVClient(fileConfig.getClamavHost(), fileConfig.getClamavPort(), DEFAULT_TIMEOUT);      

            if(ClamAVClient.isCleanReply(cl.scan(inputStream))){
                return noVirusFound;
            }
        } catch (Exception e) {
            logger.error("Error while scanning file: ", e);
            return e.toString();
        }
        return noVirusFound;
    }

From the check virus function, i have to return a string so I pass NO_VIRUS_FOUND into it from the first class.

  • Is it an acceptable practice to pass a string from one class to another just for the purpose of string comparison at the end. My fear is that if I define the strings separately in two class, change in one class would severely affect the output of the function in the other.

  • Can there could be some improvements that some one can suggest to the above situation?

\$\endgroup\$
1
  • \$\begingroup\$ But let me know how did u test it The negative scenario ? How did u check the exception? Because antivirus don't allow to download a virus file in windows \$\endgroup\$ Sep 21, 2022 at 7:06

1 Answer 1

1
\$\begingroup\$
  1. there seems to be logical falacy: what if ClamAVClient.isCleanReply() returns false but no exception is thrown? there is no else clause there, and checkVirus would return no virus found.

  2. instead of returning String, why doesn't checkVirus() re-throw the exception? that way, it can also return a String of a virus name (or whatever is produced from ClamAVClient)

  3. instead of passing the String, make it public. then any class can refer to it as FileUpload.NO_VIRUS_FOUND

  4. better still, make an enum out of the constant. that way, you can add values to the enum (like VIRUS_FOUND ...)

though it is arguable, Coding by exception is considered an anti pattern. Exceptions should be reserved for error conditions, not as indicator that a virus was found.

EDIT:

with regards to the question how to implement pt 2: First, I would create a custum exception that signifies a failed virus check

public class VirusCheckException extends Exception {
    public VirusCheckException(Throwable cause) {
        super(cause);
    }
    public VirusCheckException(String message, Throwable cause) {
        super(message, cause);
    }
    // also rest of constructors...
}

then, instead of returning the exception message, throw the custom exception. the return value is freed to return more rich data from the scan (if possible):

public static String checkVirus(FileConfig fileconfig, InputStream inputStream, String noVirusFound) 
throws VirusCheckException {
    try {
        ClamAVClient cl = new ClamAVClient(fileConfig.getClamavHost(), fileConfig.getClamavPort(), DEFAULT_TIMEOUT);      

        Object response = cl.scan(inputStream);
        if (ClamAVClient.isCleanReply(response)) {
            return noVirusFound;
        } else {
            return ClamAVClient.getVirusName(response);  // assuming there is such...
        }
    } catch (Exception e) {
        return new VirusCheckException("Virus Found!", e);
    }
}

now you will have to catch that exception in the callng method and handle it:

public void checkFile(...) {
    try {
        antiVirusResult = FileCheckClass.checkVirus(getConfig(), file.getInputStream(), NO_VIRUS_FOUND);
    } catch (VirusCheckException e) {
        generateAntiVirusMessages(e.getMessage());
        throw new ValidatorException(msgs);
    }
}
\$\endgroup\$
8
  • \$\begingroup\$ can you show me how your 2nd point can be implemented? \$\endgroup\$
    – gogogaga
    Apr 10, 2018 at 13:12
  • \$\begingroup\$ see edited answer \$\endgroup\$ Apr 10, 2018 at 13:41
  • \$\begingroup\$ in your 2nd point you mentioned 'instead of returning String' but ín your implementation you rethrow the exception and still use a string. anyways, it was very helpful. instead of passing the string from another class, i just return a 'boolean' to indicate virus check failed or successful. \$\endgroup\$
    – gogogaga
    Apr 11, 2018 at 12:00
  • \$\begingroup\$ Also, i don't need a custom exception in that case. \$\endgroup\$
    – gogogaga
    Apr 11, 2018 at 12:01
  • \$\begingroup\$ And thanks for pointing out the logical fallacy :) \$\endgroup\$
    – gogogaga
    Apr 11, 2018 at 12:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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