Re: svn commit: r1864881 - /ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java

[Mathieu Lirzin]

Hello Jacques,

[hidden email] writes:

> -            File dir = new File(imageServerPath + dirPath);
> +            File dir = new File(imageServerPath + dirPath).toPath().normalize().toFile(); // cf. OFBIZ-9973

IMO This comment should include a short rationale too.  Referencing a
bug report is really useful when the issue is complex and the reader
might want to understand the details leading to a non-obvious piece of
code, but it would be better to *not* require the developers to do the
extra work of looking at JIRA to get a broad idea of the rationale. I
would recommend the something like:

--8<---------------cut here---------------start------------->8---
// Normalize the path because we want to ensure that ... (see OFBIZ-9973 for more details).
--8<---------------cut here---------------end--------------->8---

Additionally in order to avoid repeating such comment in multiple places
creating a helper method factorizing the code related to fixing that
issue and putting the comment ontop of the method definition is nice
too.

My 2¢.

Thanks Jacques.

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[Jacques Le Roux]

Makes sense Mathieu,

I'll...

Jacques

Le 10/08/2019 à 18:43, Mathieu Lirzin a écrit :

> Hello Jacques,
>
> [hidden email] writes:
>
>> -            File dir = new File(imageServerPath + dirPath);
>> +            File dir = new File(imageServerPath + dirPath).toPath().normalize().toFile(); // cf. OFBIZ-9973
> IMO This comment should include a short rationale too.  Referencing a
> bug report is really useful when the issue is complex and the reader
> might want to understand the details leading to a non-obvious piece of
> code, but it would be better to *not* require the developers to do the
> extra work of looking at JIRA to get a broad idea of the rationale. I
> would recommend the something like:
>
> --8<---------------cut here---------------start------------->8---
> // Normalize the path because we want to ensure that ... (see OFBIZ-9973 for more details).
> --8<---------------cut here---------------end--------------->8---
>
> Additionally in order to avoid repeating such comment in multiple places
> creating a helper method factorizing the code related to fixing that
> issue and putting the comment ontop of the method definition is nice
> too.
>
> My 2¢.
>
> Thanks Jacques.
>

[Jacques Le Roux]

Done

Le 10/08/2019 à 19:29, Jacques Le Roux a écrit :

> Makes sense Mathieu,
>
> I'll...
>
> Jacques
>
> Le 10/08/2019 à 18:43, Mathieu Lirzin a écrit :
>> Hello Jacques,
>>
>> [hidden email] writes:
>>
>>> -            File dir = new File(imageServerPath + dirPath);
>>> +            File dir = new File(imageServerPath + dirPath).toPath().normalize().toFile(); // cf. OFBIZ-9973
>> IMO This comment should include a short rationale too. Referencing a
>> bug report is really useful when the issue is complex and the reader
>> might want to understand the details leading to a non-obvious piece of
>> code, but it would be better to *not* require the developers to do the
>> extra work of looking at JIRA to get a broad idea of the rationale. I
>> would recommend the something like:
>>
>> --8<---------------cut here---------------start------------->8---
>> // Normalize the path because we want to ensure that ... (see OFBIZ-9973 for more details).
>> --8<---------------cut here---------------end--------------->8---
>>
>> Additionally in order to avoid repeating such comment in multiple places
>> creating a helper method factorizing the code related to fixing that
>> issue and putting the comment ontop of the method definition is nice
>> too.
>>
>> My 2¢.
>>
>> Thanks Jacques.
>>
>

[Mathieu Lirzin]

Jacques Le Roux <[hidden email]> writes:

> Done

Thanks Jacques.

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37