Hi I'm trying to tab some code, please could someone check to make sure I have done this correctly (Changed some code for simplicity):
begin
if Password <> Database['Database']
then showmessage ('Message')
else
if NewPassword <> Retype
then showmessage ('Message')
else
begin
if Message (yes, No, etc) =yes
then
begin
List
List
List.post;
showmessage ('Message')
end
else close;
end;
end;
This is a coding style question, and may not survive here very long. :-) (Coding style is very much a matter of personal opinion, and there are an awful lot of different ones out there.) I'll give it a shot, anyway. :-)
I'd do it slightly differently. IMO, this clearly shows the proper pairings of if..else and begin..end to a new programmer:
begin
if Password <> Database['Database'] then
showmessage ('Message')
else
if NewPassword <> Retype then
showmessage ('Message')
else
begin
if Message (yes, No, etc) = yes then
begin
List;
List;
List.post;
showmessage ('Message');
end
else
close;
end;
end;
In my own code, I'd do it a little differently still (but only a minor difference). I'd move the else if password to the same line (it reduces one level of indent, and to me makes the flow of the code more clear. We have three possible options, and there are three options clearly shown (if this, else if this, else this):
begin
if Password <> Database['Database'] then // option 1
showmessage ('Message')
else if NewPassword <> Retype then // option 2
showmessage ('Message')
else // option 3
begin
if Message (yes, No, etc) = yes then
begin
List;
List;
List.post;
showmessage ('Message');
end
else
close;
end;
end;
There are only a couple of other code areas where formatting sometimes makes a difference. I'll try to quickly touch as many of them as I can think of off-hand.
Case statements:
case i of
0: DoThingForZero; // Only one line to execute for 0
1: begin // Two things to do for 1
DoSetupForOne;
DoThingForOne;
end;
2: DoThingForTwo;
else // Handle anything other than 0, 1, 2
DoThingsForOtherValues;
end;
While statements:
while not Query1.Eof do
begin
// Process each field in current record of table
Query1.Next; // Move to next row (easy to forget, infinite loop happens. :-)
end;
Repeat statements:
i := 1;
repeat
i := i + SomeFunctionResultReturningVariousValues();
until (i > 50)
For loops:
for i := 0 to List.Count - 1 do
begin
ProcessItem(List[i]);
end;
for i := List.Count - 1 downto 0 do
List[i].Delete;
For..in loops:
for ch in SomeString do // For each character in a string,
WriteLn(ch, ' = ', Ord(ch)); // write the ordinal (numeric) value
ReadLn;
Try..finally:
SL := TStringList.Create; // Create object/open file/whatever (resource)
try
// Code using resource
finally
SL.Free; // Free the resource
end;
Try..except:
try
// Do something that might raise an exception
except
on E: ESomeVerySpecificException do
begin
// Handle very specific exception
end;
on E: ESomeLessSpecificException do
begin
// Handle less specific exception
end;
else
raise;
end;
Try..finally with try..except:
SL := TStringList.Create; // Allocate resource
try
try
// Do something that might raise exception
except
// Handle exception as above
end;
finally
SL.Free; // Free resource
end;
Here's how I would format that code:
begin
if Password <> Database['Database'] then
showmessage ('Message')
else
if NewPassword <> Retype then
showmessage ('Message')
else
begin
if Message (yes, No, etc) =yes then
begin
List
List
List.post;
showmessage ('Message')
end
else
close;
end;
end;
The main differences are:
- Keeping the
then at the end of the same line that if is on
(unless there are multiple conditionals).
- Keeping
else statements in line with if statements
Personally, I'm an else if guy and also I'm against hanging then:
procedure outer;
begin {outer's}
if 2 * 2 = 4 then
begin {this belongs to if...then level}
Writeln('make bools');
end
else if Sin(X) = 3 then
begin {and this too}
Writeln('not war');
end;
else if True and SoOn then
{...}
end; {/outer's}
But in your case I'd better avoid large compound statements and heavy nesting in else branch:
begin
if not CurrentPasswordIsCorrect then
begin
Notify('bad pass');
Exit;
end;
if NewPasswordEntry1 <> NewPasswordEntry2 then
begin
Notify('you forgot your password already');
Exit;
end;
{ and so on }
end;
Sometimes the rule about writing normal flow code to then branch and abnormal to else branch becomes a false friend and inviting writing a lot of nested statements to else which in turn only hurts readability instead of improving it.
As noted in the other answers, style is highly subjective, and for that reason it is difficult to give an objective answer. However, something that can be said is how your code should look according to Embarcadero's Object Pascal Style Guide, and that is what I will attempt to answer (with some more detail than you asked for). When the same issue occurs in multiple locations, I will only mention it once.
begin
if Password <> Database['Database']
then showmessage ('Message')
Here, the casing of showmessage should be ShowMessage, which is also the name given to the procedure in the Dialogs unit. There should not be a space between ShowMessage and (.
Your use of then on the next line is unusual but okay, and you're right to indent it in that case.
else
if NewPassword <> Retype
This second if statement is part of the else branch of the first if statement, and should be indented accordingly, possibly by placing it on the same line as the else.
then showmessage ('Message')
else
begin
begin and end should not have extra indentation: the substatements should be two spaces to the right of the else above.
if Message (yes, No, etc) =yes
The fact that you have a space before the =, but not one after it, is not something I've seen before, but this is not a situation where the style guide expresses any preference, so that is okay.
then
begin
List
List
List.post;
showmessage ('Message')
end
else close;
This deserves an extra mention: else followed by a statement is explicitly called out as having fallen out of favour, but nonetheless okay, in the style guide.
end;
end;
Something else worth mentioning is that your use of two spaces for indentation is exactly what the style guide says to use.
