[SOLVED] Syntax Err ; but else found

[lucamar]

Everything everyone has said above is correct and good advice.  I just want to add a suggestion, which is, try not to have nested ifs.  They can be hard to understand and maintain.

Nested ifs, down to three levels if they are short, aren't all that bad if the code is sanely indented. The problems come when the ifs are long (more than 10 lines per alternative) or the code is badly formatted.

I have always hated the default completion for propertiy setters:

Code: Pascal  [Select][+][-]

1. procedure TSomeClass.SetSomeProperty(const Value: String);
2. begin
3.   if Value = FString then
4.     Exit;
5.   FString := Value;
6. end;

IMO it should be just:

Code: Pascal  [Select][+][-]

1. procedure TSomeClass.SetSomeProperty(const Value: String);
2. begin
3.   if Value <> FString then
4.     FString := Value;
5. end;

[rvk]

I have always hated the default completion for propertiy setters:

Indeed. That's also the way it's done in Delphi everywhere.

[440bx]

Nested ifs, down to three levels if they are short, aren't all that bad if the code is sanely indented. The problems come when the ifs are long (more than 10 lines per alternative) or the code is badly formatted.

I agree. Good (not to mention correct) indentation makes a big difference. It's quite common to need two levels.  Personally, if I have to go to three levels, I rethink the code so it can be done in two levels or less.   When I see more than 3 levels, odds are really good that code is going to be very short lived.  Of course, the more statements present in the belly of each if, the worse it gets (and the shorter its life if it's my hands.)

C/C++ programmers seem to be particularly prone to having if else if else if else ad nauseum. 

I have always hated the default completion for propertiy setters:

Code: Pascal  [Select][+][-]

1. procedure TSomeClass.SetSomeProperty(const Value: String);
2. begin
3.   if Value = FString then
4.     Exit;
5.   FString := Value;
6. end;

IMO it should be just:

Code: Pascal  [Select][+][-]

1. procedure TSomeClass.SetSomeProperty(const Value: String);
2. begin
3.   if Value <> FString then
4.     FString := Value;
5. end;

I like the other way (the one you don't like) because the statement that sets the value is not in the if statement. That way the statements are independent. IMO, the less coupling, the better.

[lucamar]

I like the other way (the one you don't like) because the statement that sets the value is not in the if statement. That way the statements are independent. IMO, the less coupling, the better.

Of course, I find my way more "clean". I guess it's a question of taste and what one is used to.

[ASerge]

IMO it should be just:

Code: Pascal  [Select][+][-]

1. procedure TSomeClass.SetSomeProperty(const Value: String);
2. begin
3.   if Value <> FString then
4.     FString := Value;
5. end;

IMHO it should be just:

Code: Pascal  [Select][+][-]

1. procedure TSomeClass.SetSomeProperty(const Value: String);
2. begin
3.   FString := Value;
4. end;

If the strings are the same, it doesn't matter which one we save. But string assignment is much faster than comparison.

[lucamar]

IMHO it should be just:

Code: Pascal  [Select][+][-]

1. procedure TSomeClass.SetSomeProperty(const Value: String);
2. begin
3.   FString := Value;
4. end;

If the strings are the same, it doesn't matter which one we save. But string assignment is much faster than comparison.

Yes, but that it's a string is purely anecdotic, just a "for example". The question is that it's (to me) more logical to compare the values (whatever type they may be) for non-equality than to compare for equality and using exit. But that is just me, my own preference: I follow better the code when there are no "gotos" (special or otherwise) interrupting the flow.

[440bx]

IMHO it should be just:

Code: Pascal  [Select][+][-]

1. procedure TSomeClass.SetSomeProperty(const Value: String);
2. begin
3.   FString := Value;
4. end;

If the strings are the same, it doesn't matter which one we save. But string assignment is much faster than comparison.

I'll make my just about total lack of knowledge about objects/classes evident. I have a question about that statement, doesn't it cause a copy of the Value string to be made ? (that would be expensive and would justify the comparison) Is making or not making a copy dependent on the $LONGSTRINGS setting ?

Thanks!

[Cyrax]

IMHO it should be just:

Code: Pascal  [Select][+][-]

1. procedure TSomeClass.SetSomeProperty(const Value: String);
2. begin
3.   FString := Value;
4. end;

If the strings are the same, it doesn't matter which one we save. But string assignment is much faster than comparison.

I'll make my just about total lack of knowledge about objects/classes evident. I have a question about that statement, doesn't it cause a copy of the Value string to be made ? (that would be expensive and would justify the comparison) Is making or not making a copy dependent on the $LONGSTRINGS setting ?

Thanks!

No. Only reference (pointer) is copied (and internal reference counter is increased), not the contents (if the $LONGSTRINGS setting is on). If  FString := Value; would be FString := Value + 'a';, then  contents of Value is copied to new string, along the 'a' character.

[440bx]

No. Only reference (pointer) is copied (and internal reference counter is increased), not the contents (if the $LONGSTRINGS setting is on). If  FString := Value; would be FString := Value + 'a';, then  contents of Value is copied to new string, along the 'a' character.

Thanks Cyrax.  I appreciate the explanation.   

It makes me think that, just maybe, the reason they perform the comparison is as a performance measure in case $LONGSTRINGS is off (which is probably rare now but, they may be "playing it safe".)