Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#4984 closed Task (wontfix)

Some statements are comma separated

Reported by: Alfonso Martínez de Lizarrondo Owned by:
Priority: Normal Milestone:
Component: General Version: 3.1
Keywords: Discussion Cc:


Looking at the code in plugins/undo.js I've found two statements like

	return this.restoreImage( image ), true;

That ", true" is useless as far as I understand and the lint check doesn't give a warning because it's disabled in its config:

-comma_separated_stmts        # multiple statements separated by commas (use semicolons?)

Enabling that shows that there are other places where such problem exists. Some of them I guess that can be fixed by changing the comma to semicolon, but I don't know what's the logic for other situations like this one in tabletools:

	if( trimCell( cell ), cell.getChildren().count() )

maybe it's a && there?
But as that can change the behavior each change should be done knowing what's the logic that should apply.

Change History (5)

comment:1 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Discussion added
Type: BugTask

The comma can be used to execute more than one statement, returning the results of the last one. It can just make some code blocks smaller.

In the first case, probably you have also an if there:

if ( something )
    return this.restoreImage( image ), true;

Instead of:

if ( something )
    this.restoreImage( image );
    return true;

It doesn't really impact in the code readability, making it in effect more compact. At the same time it also saves 2 bytes in the final output (but this is relatively important).

In the second case, we could easily have this:

trimCell( cell );
if ( cell.getChildren().count() )

Here, the comma impacts on the code readability, and we should really change it. We not even have any impact in the output size.

So, as we can see, this is something changeable. It's just a matter of considering the benefits of having shorter statements, and then define the rule to whether use it or not.


comment:2 Changed 8 years ago by Alfonso Martínez de Lizarrondo

As you can see from my initial post I didn't expect this behavior and I really thought that it was just some dead code as this kind of coding isn't usual.

For me it's easier to understand the "expanded" syntax, but I'm just one man and for example I don't like either the jQuery syntax, despite all the people that love it.

comment:3 Changed 6 years ago by Jakub Ś

Resolution: wontfix
Status: newclosed

I think we can close this task.

Here, the comma impacts on the code readability, and we should really change it.

I don't agree here. Sure this is not the syntax you see every day but imagine the situation where some beginner is saying that he does not understand syntax for ternary operator and claims that it should be replaced with if else statements. You would never go for something like that:)

If it doesn't impact anything and makes the code smaller that IMO this is just a matter of learning this technique.

comment:4 Changed 6 years ago by Alfonso Martínez de Lizarrondo

I'm not going to argue, but making the code smaller should be a task for the packager, and the source code should be easy to read and understand.

I would link this topic to the "Thoughts about coding style" thread. It would be interesting to know the position of someone that hasn't touch that code and if he has saw this pattern used elsewhere.

comment:5 Changed 6 years ago by Jakub Ś

position of someone that hasn't touch that code and if he has saw this pattern used elsewhere

@alfonsoml - most likely the answer will be no. You are 100% right that this is something that you don’t see every day.

All I’ve meant is that this is everyday programmer’s life :) you don’t know some syntax or pattern one day, you learn it the other. When I saw first this post it also had me thinking for a moment but after reading @fredck explanation I just thought – cool, does the same and takes less space. Perhaps it’s just a matter of adaptation/point of view :).

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy