New "figures" feature is an excellent addition, thanks Paul and Mattias!

It already motivated be to fix some stylistic problems in one of my projects ;-)

A few questions/suggestions for improvement:
1) Where did the name came from? It is totally meaningless for me. Maybe 'Stylistic notes' or 'Style' or something along these lines?
2) What is wrong with published properties without default?

3) I also like that codeexplorer options are now integrated with the common dialog, however:
3a) Maybe 'Enable figures' checkbox should be moved to or duplicated on the 'Figures' page?
3b) Maybe option 'parameters', lice line/procedure counts, should be placed near the corresponding checkboxes?
3c) What is the reason for 'search char constants' option? I think it should just be always on.
3d) I think '' (empty string) should be added to the default list of ignored constants.

4) A few wishes/feature requests:
4a) 'Empty procedures' should not trigger if the procedure has comment in the body,
to take care of a common idiom:

  procedure TMyClass.Proc;
  begin
    // do noting
  end;

4b) Add 'Unsorted procedures' similar to 'unsorted members' 4c) Add 'Empty begin..end' block, probably with the same check for the comments as in (4a)
4d) Add 'Single statement in begin..end block'
4e) Add 'Too many nesting statements' and 'Too many nesting procedures' 4f) Add 'Line loo long' with default limit of perhaps 80 or 90 characters. 4g) Add 'Incorrect indentation' for the cases where indentation does not correctly represent
statement nesting.
4h) Add 'Fixme' similar to 'TODO'
4i) This is probably an unrelated issue, but 'TODO' is only recognized inside multi-line {} comments,
not inside single-line (//) ones.

See
http://www.askoxford.com/concise_oed/figure?view=uk

Maybe 'Stylistic notes' or 'Style' or something along these lines?

Not all are about style.

2) What is wrong with published properties without default?

See
http://wiki.freepascal.org/User_Changes_2.2.4#Default_property_values

If a property is streamed without need it
1. increases the size of the lfm
2. makes updating error prone, because even useless properties are streamed. For example if a property was introduced and later removed/renamed it is still in the lfm and the compiler does not notice.

The LCL had the last case a few times in the last years and every time you have to check all lfm files. Therefore the LCL has a built in list of ignored properties.

3) I also like that codeexplorer options are now integrated with the common dialog, however: 3a) Maybe 'Enable figures' checkbox should be moved to or duplicated on the 'Figures' page?

Yes. But I'm biased about duplicate or move.

3b) Maybe option 'parameters', lice line/procedure counts, should be placed near the corresponding checkboxes? 3c) What is the reason for 'search char constants' option? I think it should just be always on.

IMO char constants like #10, #13, 'a' or ' ' don't need to be listed.

3d) I think '' (empty string) should be added to the default list of ignored constants.

IMO no one wants to see empty string constants in this list, so no need for an option.

The figures are hints, not warnings.
I already found with this feature some empty methods with comments that were useless and could be removed.

4b) Add 'Unsorted procedures' similar to 'unsorted members'

I rarely have seen units where all procedure are sorted alphabetically. Same for enums, types, variables and constants.

Are you joking or are you a grammar nazi?

4h) Add 'Fixme' similar to 'TODO'

AFAIK the todo list is not yet customizable. If you extend it, the code explorer will be extended too.

4i) This is probably an unrelated issue, but 'TODO' is only recognized inside multi-line {} comments, not inside single-line (//) ones.

Fixed.

Mattias

Alexander Klenin wrote

4f) Add 'Line loo long' with default limit of perhaps 80 or 90 characters.

Not a bad figure to add. I would choose only bigger limit - 100-120 chars.

Best regards,
Paul Ishenin.

Mattias Gaertner wrote

> A few questions/suggestions for improvement: > 1) Where did the name came from? It is totally meaningless for me. See http://www.askoxford.com/concise_oed/figure?view=uk

Hm. I'll quote that:

Not one meaning correspond to 'a hint about possible problem in the code'.

> ? Maybe 'Stylistic notes' or 'Style' or something along these lines? Not all are about style.

At least it will give user a vague idea what it is ;-) How about simply 'hints'?

These are all good arguments, but _against_ the removal of 'default default' property values. IMHO the FPC's team decision is wrong, but I think it is too late ;-(
Now I have to to bloat my code with 'default false' lines, which were not necessary before.
I see the reason for the hint now -- but I think it should be made a FPC warning then.

Maybe, but how about #34 or '?'? Char constant may be just as magical as any other ;)

But it _is_ shown now.

> 4b) Add 'Unsorted procedures' similar to 'unsorted members' I rarely have seen units where all procedure are sorted alphabetically. Same for enums, types, variables and constants.

Well, same for methods and properties.
Does LCL have one unit where 'unsorted members' list is empty? Still, this list is IMO useful and so it is for procedures.

Well, 'grammar nazi' can not exist for a compiled language, since compiler will simply reject grammatically incorrect source. So you probably mean 'stylistic nazi'. Well, yes, actually I am ;-)

However, the checks I suggested are very far from 'nazi' level. Truly nazi-checks would include things as 'no double empty lines', 'all local variables start from lower case', 'begin statement never/always alone on the line',
'no underscores in identifiers', '"try.. finally..Free..end" for each locally created object'
and many, many others.

The ones I suggested are in fact very conservative and some are in direct analogue with already existing ones:
"Empty begin..end" <=> "Empty class section"
"Too many nesting levels" <=> "Too long procedure/Too many nested procedures".

"Incorrect indentation" is IMO hard to implement, but it will probably be the most useful
of all checks, since it will with 99% probability indicate an actual bug (even if merely the code formatting bug).

> 4h) Add 'Fixme' similar to 'TODO' AFAIK the todo list is not yet customizable. If you extend it, the code explorer will be extended too.

Fair enough.

> 'TODO' is only recognized inside multi-line {} comments, > not inside single-line (//) ones. Fixed.

Thanks.

Hm, while some of them probably are most likely to be used to find possible issues, there is no need to see them as an "problem indicator" only.
So point 1 is "correct" as they are a collecion of numbers "figures"

Still, maybe they would be better named as "statistics"? (maybe "analytics" ?)

Alexander Klenin wrote

However, the checks I suggested are very far from 'nazi' level. Truly nazi-checks would include things as 'begin statement never/always alone on the line',

Funny that you think that is extreme. begin always alone on a newline, is about the only convention I insist on.

I would call them "IDE Hints" or "Code Hints". But "Figures" are also ok for me.

Best regards,
Paul Ishenin.

The problem with this particular convention it that it is probably the hottest holy war topic in all Pascal stylistics discussions. For example, I (and company I work at) insist on the opposite ;-) So if it is ever implemented, it should include 'always/never' switch.

My main point with the name is usability. The name should convey at least something about the named object to an unprepared reader -- this is an important rule both in programming and interface design. So yes, 'code hints', 'analytics' -- anything suggested in this thread is better than 'figures'.
(Not 'statistics', however, which, by the way, would be another interesting feature)

I thought about 'statistics', 'hints' and some more and IMO they are all misleading.
The new section should sum up and/or list all kinds of noteworthy numbers, places, lines, structures and facts of the current unit, that don't fit into the other categories.
These combine several different types of categories like: -unoptimized code/left over: e.g. empty methods
-code formatting: e.g. unsorted members
-readability/needs refactoring: e.g. long procedures, unnamed constants -comments: e.g. todos
-...

About 'statistics': statistics are about summarizing and interpretation. None of the current categories fit into this description.

About 'hints':
Theoretically you can define 'hint' very generic: some message about a pascal source. But then the term 'hint' becomes almost meaningless and every time a user user reads about 'hint' he doesn't know whether it means fpc hint or lazarus hint. Just like the word 'package' has more than five meanings on this mailing list. IMHO the term 'hint' has already enough meanings.

Modelmaker has a similar feature called 'live metrics', which fits, because they mostly list code formatting and readability categories.
But IMO the category 'code formatting' is questionable. All other categories list things, that need some manual attention - the programmer must decide if and how to change it.
OTOH 'code formatting' can be applied safely.
If you don't like long lines, then you need a tool to break them. When such a tool exists, you don't need a tool to show where the long lines are, you just need a flag: this unit needs code formatting. Instead of adding code explorer categories 'unsorted procedures' it makes more sense to improve the jedi code formatter and its lazarus integration.
In fact, I added the 'unsorted members' categories just as demonstration and speed test.

Another argument for 'figures' is this:
At the moment the items are listed in the tree.
Eventually the lists should be moved to a second tree below, where some context buttons are shown. Additionally this would waste less space and update time.
Then the current tree will only list the available categories and the total numbers, which is afaik often called in English 'figures'.

Mattias

Then you have never seen mine. In order to find way in my code I tend to sort all my methods.

Marc

Paul Ishenin wrote

Alexander Klenin wrote: > 4f) Add 'Line loo long' with default limit of perhaps 80 or 90 characters. > Not a bad figure to add. I would choose only bigger limit - 100-120 chars.

What about right margin setting ?

Marc

See my other mail why not using 'hints'.

Both ways are not optimal, but now it is at least Delphi compatible.

I see the reason for the hint now -- but I think it should be made a FPC warning then.

A warning will annoy more than it helps.

That's why it is optional.

Where?

True. Although private sections are mostly created by class code completion, which sorts.

Does LCL have one unit where 'unsorted members' list is empty?

IMO alphabetical sorting makes only sense if you have no obvious logical sorting, OR if you prefix every member, so that alphabetically and logically sorting produce the same results.

Still, this list is IMO useful and so it is for procedures.

See my other mail why such things are better be done elsewhere.

;)
And you want the IDE to be a stylistic nazi like yourself.

Well in some way I understand you. I don't like macros and I will eventually add a category to list all macros.
But IMO it is more important to provide a tool to help fixing these things. Which means for the examples you gave ...

[...code formatting like indentation, empty or long lines...]

... a code formatter.

Mattias

:)

For me alphabetically sorting is like the index at the end of a book. Logical sorting is like the directory at the beginning of a book.

Both are useful, although with current search tools you hardly need the index anymore. The problem with logically sorting is that it is still hard to implement.

Mattias

Mattias Gaertner wrote

Theoretically you can define 'hint' very generic: some message about a pascal source. But then the term 'hint' becomes almost meaningless

But you propose to define 'figure' as an even more generic thing: "number, place, line, structure or fact", so it will have even less meaning.

every time a user user reads about 'hint' he doesn't know whether it means fpc hint or lazarus hint.

And this is not a bad thing IMO -- these hints could just as well be in FPC, since their usefulness has nothing to do with Lazarus itself. FPC just moves too slowly.
Conversely, it would be useful to display FPC hints in the same list and with the same grouping.

We need a native English speaker to resolve this ;-) Yes, 'figures' may mean 'totals', but this word does not convey anything at all to the user who does not already know they are actually hints. You can call them 'things', 'items' or 'jabberwacks' -- it will not make any difference.

Please note that stylistic-nazi examples I gave are different from the checks I proposed to add. One difference, which I considered while making the latter list,
is whether code formatter will be able to fix it.
Let's review:
(4c) and (4d) are in principle fixable automatically, but the tool to do that should definitely not be called 'formatter'.
(4e) has nothing to do with formatting
(4f) seems like a no-brainer, but it has issues too:
1) there are times when slightly overlong lines are the lesser of two evils, like e.g. DebugLn lines in Lazarus or some list-formatted array initialization.
2) line splitting, sepecially for complex expression, seriously affects readability,
so the choice of the optimal line break position can not be automated. (4g) again seems simple, but 'auto-fixing' can in this can make things _worse_ by hiding the problem. Consider the code (idented by '~' characters): for i := 0 to High(A) do
~~if A[i] > m then
Writeln(A[i]);

The actual error is a missing 'm := i' line after the if, but
formatter would change it to
for i := 0 to High(A) do
~~if A[i] > m then
~~~~Writeln(A[i]);

Obtaining correctly formatted but semantically wrong code. Hint, on the other hand, will simply point programmer to the problematic location
where he will hopefully notice and fix it.

I normally group mine according to purpose, so if there's a collection of procedures for handling a particular object, they will be together, etc.
I realise that by altering the naming convention, I could have both logical and alphabetical sorting, but that could end up with some very convoluted procedure names.. :shrug:

DSP

Maybe it should be 'Go figure(s)'...

So what have we got so far?
Figures
Statistics
Style
Hints
Analytics
Metrics
Formatting
..

Well, it's kinda all of them.. Whilst it isn't a very 'hip and happening' term, it strikes me that they are Observations about the code structure, formatting, etc. Maybe 'Code Observer' would suit?

DSP

No you are not. You gave some valid points that the categories are not stylistic. See below.

[...] Let's review: (4c) and (4d) are in principle fixable automatically, but the tool to do that should definitely not be called 'formatter'.

About (4c) - empty begin..end
I think it should be added, including repeat..until, try..finally, try..except, case..else, but only when there is no comment.

About (4d) - single line begin..end
Maybe you can give an example when this is not about code formatting?

(4e) has nothing to do with formatting

True

A good formatter is very flexible and has a lot of options to fine tune indentation and line break. IMO complex expressions which can not be automatically broken nicely, should be changed anyway. Maybe the code explorer should show deeply nested expressions.

Long lines depends on right margin and this heavily depends on programmers choice. For example if you set your right margin to 80 and you open a unit of other programmers the code explorer will easily list thousands of long lines. A performance killer.

Ok, the above example is a good. I thought you meant showing wrong indentation in general. That would not make much sense, because the indentation rules belongs to the code formatter and makes only sense for units you wrote.
So the code explorer can scan for lines 'do' and 'then' lines where the following line is indented less or equal. Of course some programmers will switch off the "equal".

Mattias

I like that. It fits and is unique enough. :)

Mattias

while a nice idea, even this is not "that" easy

~~if a = 1 then
~~begin // if it should be on it's own line
end;

1) the begin is *or* is not expected to be intended, so the rule above should carter for that?
2) the end should be aligned with the start of the line, that did have the begin

~~if (a = 1) and // some very long condition or comment ~~~~~(b=2) then
~~~~writeln;

Imho is correctly indented, because the then is already moved in. (in fact the 2nd condition line(5) is intended more (aligns the "(" ) than the "writeln" line(4))

if a = 1 then b:=2;
writeln(b);
if a = 1
then b:=2;
writeln(b);

the line continues behind the "then" => no indent is expected.

if a = 1 then b:=2 else
writeln(b);

=> I had no idea what indent was expected here?

An embedded and charset-unspecified text was scrubbed... Name: not available
Url: http://www.lazarus.freepascal.org/pipermail/lazarus/attachments/20090420/9bdbc357/attachment.pl

Mattias Gaertner wrote

About (4d) - single line begin..end Maybe you can give an example when this is not about code formatting?

I just mean that removing non-whitespace characters is not formally 'formatting'.
I agree it is gray area.

A good formatter is very flexible and has a lot of options to fine tune indentation and line break. IMO complex expressions which can not be automatically broken nicely, should be changed anyway. Maybe the code explorer should show deeply nested expressions.

Or simply expressions with too many tokens, regardless of nesting.

Long lines depends on right margin and this heavily depends on programmers choice. For example if you set your right margin to 80 and you open a unit of other programmers the code explorer will easily list thousands of long lines. A performance killer.

Is not it O(total number of lines) anyway? I do not see how a performance could depend on that. If you are worried about the number of items found, then I suggest to implement a generic limit of, say, 100 items. 100'th item should contain 'too many items found' text.

How about this general rule: foldable blocks content should be indented? Some exceptions are necessary, of course.

FYI: The jedi code formatter can add semicolons.

You can create a lot of tokens without getting complicated:

if (Nodenil)
and (Node.TheType=something)
and (Node.Parentnil)
and (Node.Parent.TheType in [bla,bla,bla])
and (Node.Parent.NextBrothernil)
then

Reading is far less expensive than creating a TTreeNode with an icon, a text and data. And don't forget that the tree nodes must be sorted.

If you are worried about the number of items found, then I suggest to implement a generic limit of, say, 100 items. 100'th item should contain 'too many items found' text.

Yes. But we have already more than ten categories and I guess we can easily get twenty.
I guess the only solution is to parse only til the first occurence and read the rest only on demand.

Mattias