On 2013-07-03 11:58, David Mason wrote:
> In zanata-war/src/main/java/org/zanata/rest/service/VirusScanner.java:
> 
>> +         int exitValue = executor.execute(cmdLine);
>> +         log.debug("{} to scan file: '{}'", stop, documentName);
>> +         switch(exitValue)
>> +         {
>> +         case 0: // clamscan: 0 : No virus found.
>> +            log.info("{} says file '{}' is clean: {}", scannerName, documentName, output);
>> +            return;
>> +         case 1: // clamscan: 1 : Virus(es) found.
>> +            throw new VirusDetectedException(scannerName + " detected virus: " + output);
>> +         case 2: // clamscan: 2 : Some error(s) occured.
>> +         default:
>> +            // This can happen if clamdscan is found, but the clamd service is not running.
>> +            String msg = scannerName + " returned error scanning file '"+documentName+"': " + output +
>> +               (usingClam ? "\nPlease ensure clamd service is running." : "");
>> +            throw new RuntimeException(msg);
>> +         }
> 
> Readability depends on the syntax highlighting settings, and I would
> argue that the code should ideally be readable in all the forms we're
> likely to look at it, including as part of a diff such as the above.

That's true, in fact I just suggested to pahuang to keep lines under 100
chars due to github's diff display (but only as a future suggestion).

The real question is, why does github limit the width of the diff
column, and why don't we have syntax highlighting diffs yet ;-)?

> 
> I agree that since the numeric values are now used only once it is less
> sensible to move them to statics. There might still be a case for
> explanatory local variables, since they would be easy to maintain with
> the switch statement and would provide better separation of concerns
> (e.g. which int happens to mean a virus was detected, vs. what to do
> when a virus is detected).

Fair enough, I did consider locals, but I guess that's not a bad
argument for using them here.  I'll make the change.


> —
> Reply to this email directly or view it on GitHub
> <https://github.com/zanata/zanata-server/pull/11/files#r5000698>.
> 


-- 
Sean Flanigan

Senior Software Engineer
Engineering - Internationalisation
Red Hat