Fwd: Re: [icedtea-web] Regression after refactoring for XDG specification

[Jiri Vanek]

Hi!

Can i push? This patch will be soon inapplicable, and it is heavily needed.


J.

-------- Original Message --------
Subject: Re: [icedtea-web] Regression after refactoring for XDG specification
Date: Thu, 12 Sep 2013 11:40:01 +0200
From: Jiri Vanek <jvanek at redhat.com>
To: Jacob Wisor <gitne at gmx.de>
CC: distro-pkg-dev at openjdk.java.net

Ok, painfull review indeed!

I did my best, see below:

On 09/10/2013 09:34 PM, Jacob Wisor wrote:
> Jiri Vanek schrieb:
>> On 07/30/2013 06:14 PM, Jacob Wisor wrote:
..snip...
>>> Nevertheless, thank you for addressing this issue, so please keep up the good work.
>
>> +    private static void ensureMainDirs() {
>
> I am especially unhappy with this method's naming. I am aware that naming is a
> difficult task in CS, but it pays off investing time into it. What does it
> ensure main directories about? What happens if something cannot be or has not
> been ensured? Does possibly a RuntimeException be thrown? Except from that, what
> are main directories supposed to be? Perhaps root directories? What makes a
> directory so distinct to be "main"? So, please find an adequate name and add
> some documentation.

Ok. I refactored it a bit and added javadoc. I hope it is much cleaner now. Thanx for well deserved
kick.

>
> Some language stuff:
>> +                    System.out.println("WARNING: key " + key + " do not have
> value, switching to default");
>
> "WARNING: key " + key + " has no value, setting to default value"
>
>> +                    System.out.println("WARNING: key " + key + " do not have
> value, skipping");
>
> "WARNING: key " + key + " has no value, skipping"
>
>> +                    System.err.println("ERROR: key " + key + " value: " +
> value + " not existed, and was NOT created");
>
> "ERROR: Directory " + value + " denoted by key " + key + " does not exist and
> has not been created"
>
>> +                    System.out.println("OK: key " + key + " value: " + value
> + " not existed, and was created");
>
> "OK: Directory " + value + " denoted by key " + key + " did not exist but has
> been created"

Used. Thank you.
>
>> +    //not private for testing
>> +    static String testMainDir(File f, boolean verbose) {
>
> Since you have made this method not private please add some documentation on
> what this method's purpose is and what it does. It is definitely not
> self-explanatory, even after reading its code. And, as mentioned below it is
> probably unnecessary complex and seems to be mixing functional code with message
I hope fixed by refactoring.

> generating code. If you really want to keep this differentiated error reporting
> feature, I would strongly advise to deglomerate functional and message
> generating code.

Well, nothing else then pure laziness was on my side here:( Sorry!  I have added wrapper class which
is keeping the symptoms of failures, and message is generated later, based on those flags.

>
>> +        StringBuilder messages = new StringBuilder();
>> +        if (!f.exists()) {
>> +            String s = (R("DCmaindircheckNotexists", f.getAbsolutePath()));
>> +            if (verbose) {
>> +                System.err.println(s);
>> +            }
>> +            messages.append(s).append("\n");
>> +        }
>> +        if (!f.isDirectory()) {
>> +            String s = (R("DCmaindircheckNotdir", f.getAbsolutePath()));
>> +            if (verbose) {
>> +                System.err.println(s);
>> +            }
>> +            messages.append(s).append("\n");
>> +        }
>
> These two tests are redundant. File.isDirectory() implies a check for existence.
> It does not matter to the user or the application whether the desired directory
> does not exist or is not a directory. In effect the program cannot read
> from/write to the desired location.

Partially yes, and aprtially no. IsDirectory really implies fileExists, but the message then would
be misleading. I really would like to keep those most common error cases separately.
>
>> +        File testFile = null;
>> +        boolean ok = false;
>
> Naming again. What is ok?

renamed.
>
>> +        try {
>> +            testFile = File.createTempFile("maindir", "check", f);
>> +            if (testFile.exists()) {
>> +                ok = true;
>> +            }
>> +            try {
>> +                FileUtils.saveFile("ww", testFile);
>> +                String s = FileUtils.loadFileAsString(testFile);
>> +                if (!s.trim().equals("ww")) {
>> +                    ok = false;
>> +                }
>
> This test misses probably its point because it is testing for creating a *file*
> not a *directory*. Most file systems with DACLs distinguish between /creating/
> files or directories, and between /creating/ and /writing/ into files or
> directories, so it is important to be precise here. Furthermore, it tests
> writing into a file instead of perhaps writing into a file in a subdirectory.

Fair enough. I forgot about this case utterly. The test is now testing also subdirectory artificaly
created inisde real directory (call to testMainDir one more times)
>
>> +            } catch (Exception ex) {
>> +                if (JNLPRuntime.isDebug()) {
>> +                    ex.printStackTrace();
>> +                }
>> +                ok = false;
>> +            }
>
> No really, you should *not* catch arbitrary exceptions. It ain't just some empty
> words.

Well.. There are cases where it brings moore good then wrong. Endless loops are very often
implemented like:
public void run(){
while (true){
try{
}catch(Throwable r){
log(t)
}
}
}

By otherwords, when exception should not , by no means, disturb the program - eg my case above - it
does not sound so bad...

I would like to keep the exception cough as it is.
>
> After all, this method is overly complex and does not add any value for the
> user. So I would advise you to discard this method.
>
>> diff -r 79bdc074df81 netx/net/sourceforge/jnlp/resources/Messages.properties
>> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties    Fri Aug 30
> 11:02:08 2013 -0400
>> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties    Tue Sep 03
> 13:56:53 2013 +0200
>> @@ -304,6 +304,9 @@
>>  DCInternal=Internal error: {0}
>>  DCSourceInternal=<internal>
>>  DCUnknownSettingWithName=Property "{0}" is unknown.
>> +DCmaindircheckNotexists=After all tryes your configuration directory {0} do
> not exists
>> +DCmaindircheckNotdir=Your configuration directory {0} is not directory
>> +DCmaindircheckRwproblem=Your configuration directory {0} can not be
> read/written properly
>
> Although it is nice of you to make precise distinctions between error types to
> help the user it is actually not necessary and does not offer any additional
> help when analyzing this problem. All these messages can be fused into one

I really think that two most common cases file instead of dir and insufficient privledges  should be
distinguished.

Unless you stongly insists, I would like to keep the three messages as it was.
> sufficiently explanatory error message: DCCannotAccessConfig="Cannot access
> configuration in file {full path to config file}." This should probably be
> enough for an admin, even for a user. I would really advise this because it
> makes the code simpler and yet serves its purpose.

Well.. eh.. yes.m I admit that the code is complex. Now it is even a bit more complex. I hope that
refactoring made it much more readable, and taht testing is strong enough.

I would like to keep the "three distinguish messages" but any idea to keep them, check all parts,
and make the check algorithm a bit simpler is welcomed.
>
>
Thank you very much,
    J.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: xdgForcingDirs4.patch
Type: text/x-patch
Size: 29967 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20131022/e217de8c/xdgForcingDirs4-0001.patch