Re: [WebERP-developers] CustomerInquiry.php W3C html code.

[Phil D. <ph...@lo...>]

Hola Rafael,

As we have corresponded off list, I know that these comments within will 
be no surprise to you - but for the benefit of anyone else.


On 16/09/14 07:31, Rafael Chacón wrote:
> Hi All,
>
> *** ABOUT CustomerInquiry.php: ***
>
> 1. The problem: In some web-browser, incomplete table-rows 
> ("<tr></tr>" tags with less quantity of columns) causes an unexpected 
> behaviour (e.g. blanks and misaligned columns). E.g. Android 
> 3.0.31/Navigator (Personal comment: I promote this ERPs as an ERP that 
> can be run in a small device and can be run anywhere with a slow 
> Internet connection, therefore it must run fine and fast).
>

Well I am not sure all the screens are so good on smaller devices.


> 2. The goal: fix this problem, document it, and have a code easy to 
> understand for a newcomer (Personal comment: the last is the most 
> difficult task. We can not "reset" our memory about the code to test 
> if it is easy to understand for a newcomer).


Yes it is subjective - but consistency with existing code and strict 
adherence to the conventions/style as per

http://www.weberp.org/CodingConventions.html

should cover it.

>
> 3. What I did: I decomposed the code to understand it. After that, I 
> reassembled. The result is one section with the constant part [format 
> string and other components like html-links and called parameters 
> (names) ]; that is BaseInfo, CreditInvoice, Allocation, 
> PreviewInvoice, PreviewCredit and GLEntries section. And an other 
> section with the variable part ("parameters"), where we can see the 
> table-row structure. The main problem is still make evident the HTML 
> table structure.
>

The problem with that though is that is that it ended up with a LOT of 
changed code and the actual fix was obscured. Much better I think to 
evolve with just the code necessary to effect the bug fix


> 4. Secondary effects:
>
> 4.1. Regroups and comments the initial basic lines of script 
> (includes/session.inc, $Title, $ViewTopic, $BookMark, 
> includes/header.inc). Even without much impact, the purpose is to 
> ensure that these lines are uniformly in all scripts that require it.
>

All great contributions these.


> 4.2. Deletes $tableheader variable. This variable is defined in line 
> 253 and only used in next line (line 271). Nowhere else is used that 
> variable. I think it's better to remove it to simplify the code and, 
> theoretically(*), to become more fast the code.
>

Yep - if used several times it messes with the table sorting javascript 
anyway.

> 4.3. Regroups some echo string functions. Instead of some consecutive 
> echo language constructors, regroup them in only one instruction to 
> simplify code. I think it makes the code easier to understand.
>

Me to - probably reduces the work of the parser too.

> 4.4. Adds the ascending class to Data, Type, Number and Reference 
> columns. I found that some people look for an operation (line) seeking 
> it by transaction date (over half of the time), type, number and 
> reference (by customer's purchase order reference or by payment 
> reference). This is a new feature to help users, and it is a lack if 
> we compare CustomerInquiry.php with SupplierInquiry.php

Yep  worthwhile for most scripts
>
> 4.5. $RootPath and $Theme in the "Constant Section". Those variables 
> always hold the same values ​​throughout the time this script runs. 
> For clarity, I think it is better to separate those "constant 
> variables" from the each row variables (printf parameters section).
>

Not quite sure what you mean here?

> 5. Help wanted:
>
> 5.1. "Constant Section" variable names. Because those variables 
> (BaseInfo..., CreditInvoice..., Allocation..., PreviewInvoice..., 
> PreviewCredit... and GLEntries...) have more work than only format the 
> string between the table-datacell tags ("<td></td>"),  I "feel" that 
> the suffix "FormatString" can conduct to an error. Comments? Suggestions?
>

Can't these variables be made to have the correct number of cells 
("<td></td>") ? My impression is that this is all that is required to 
fix this script?

> 5.2. "TD..." suffix. We need to show evident the quantity of columns 
> in a table-row (pairs of "<td></td>" inside a "<tr></tr>"). I am not 
> happy adding "_TD..." suffix, but I can not think of another idea. 
> Comments? Suggestions?

I wasn't keen on the TD suffixes and initially didn't get what they 
were. Better off just to make the variables have the correct number of 
cells.
>
> 5.3. Any suggestion to  have a code easy to understand for a newcomer.
>

Follow

http://www.weberp.org/CodingConventions.html

rigorously. Of course this is just my take on this and the way much of 
the code is laid out as far as possible. If some of it isn't then 
changes to bring it in line are most welcome.

Every so often someone comes up with a bright idea to change the way the 
code is.

I am not interested re-writing the code a different way.

I am interested in bug fixes and new functionality.

However, I took Andrew's point that abstracting some code to functions 
would make some changes easier and use less characters and there is a 
middle ground with abstraction. I think we have a reasonable compromise 
- and I make no apology for erring on the side of readability - it is a 
core goal.

Phil