Code improvement, suggestions please

[cris75]

Hi all!
This is my code, I use it to save (in append mode) the output from an external process (TProcess)
My goal is to "append" to the log file, so I need to insert a newline into the FileStream, I do this in a way I'm not really convinced is the better one, or if it is safe, even if it works;
at the same time i want to store the buffer content in a string (or Result in this case) to display it (the last buffer) somewhere later in the program
What do you suggest? I mean, it works but.. do you consider it good code? Or ugly one? How could I improve it in that case?
I don't want ask IA (that way I wouldn't learn anything useful/new)
Thanks

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

1. function SaveTheFunnyLog:string;
2. var
3.   Buffer: array[0..2048 - 1] of byte;
4. ...
5. ...
6.   aProc:= TProcess.Create(nil);
7.   aProc.Options:= [poUsePipes];
8.   aProc.Execute;
9. ...
10. ...
11.   //Create a FileStream to save (append) to the LOG
12.   LogFName:= ExtractFilePath(ParamStr(0)) + 'somefile.log';
13.   if FileExists(LogFName) then begin
14.     aFs:= TFileStream.Create(LogFName, fmOpenReadWrite or fmShareExclusive);
15.     aFs.Seek(0, soFromEnd); //Move to the end of the file
16.   end else
17.     aFs:= TFileStream.Create(LogFName, fmCreate or fmShareExclusive);
18.  
19.   OutputStream:= TMemoryStream.Create;
20.  
21.   repeat
22.     BytesRead:= aProc.Output.Read(Buffer, BUF_SIZE);
23.     OutputStream.Write(Buffer, BytesRead)
24.   until BytesRead = 0;  // Stop if no more data is available
25.  
26.   aProc.Free;
27.  
28.   OutputStream.Position:= 0; //Make sure all data is copied from the start
29.   S:= LineEnding;
30.   aFs.CopyFrom(OutputStream, OutputStream.Size);
31.   aFs.WriteBuffer(Pointer(S)^, Length(S));
32.   aFs.Free;
33.   OutputStream.Free;
34.  
35.   S:= '';
36.   SetLength(S, Length(Buffer));
37.   Move(Buffer[0], S[1], Length(Buffer));
38.   Result:= S;
39. end;
40.  

[Thausand]

How could I improve it in that case?

Have some idea. Take what want, not all good but suggest. Have read and think problem in other way. One sure suggest: make use exception handle (try..finally).

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

1. function SaveTheFunnyLog:string;
2. var
3.   outputstring:string absolute result;
4.   logfilemode:word=fmShareExclusive;
5.   logfilename:string='somefile.log';
6.   logfile:TFileStream;
7. begin
8.   outputstring:='';
9.   if RunCommand('TheFunnyProgram', ['commands'], outputstring, [poUsePipes]) then
10.   begin
11.     logfilename:=ExtractFilePath(ParamStr(0))+logfilename;
12.     if FileExists(logfilename) then logfilemode:=logfilemode+fmOpenReadWrite else logfilemode:=logfilemode+fmCreate;
13.     logfile:=TFileStream.Create(logfilename,logfilemode);
14.     try
15.       if logfile.Size>0 then logFile.Position:=logfile.Size-1;
16.       logfile.WriteBuffer(outputstring[1], Length(outputstring));
17.       WriteStr(outputstring, LineEnding);
18.       LogFile.WriteBuffer(outputstring[1], Length(outputstring));
19.     finally
20.       logfile.Free;
21.     end;
22.   end
23.   else Error;
24. end;
25.  

Code my example is make same approach you example. Good implementation is depend situation my example no except. I have use simple runcommand and no complicate TProcess (you example no show is need but is work same when want) and then no need buffer read/write and have memorystream for that (make more simple)

PS: make wish Pascal some day can have set value default for return.

[AlexTP]

>>I don't want ask IA (that way I wouldn't learn anything useful/new)

IA == illegal alien? Mexican migrant on Trump's borders. Weird.

[Khrys]

IA == illegal alien? Mexican migrant on Trump's borders. Weird.

OP most likely speaks a Romance language, where adjectives frequently come after the noun they modify - e.g. "artificial intelligence" is "intelligence artificielle" in French.

[cdbc]

Hi
@cris75: Your code looks just fine to me, when it works, I see no reason to change it 
Regards Benny

[Thaddy]

What is so difficult and complex to log?

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

1. const
2.   logfilename = 'mylog.txt';
3.  
4. procedure log(const S:string);
5. var
6.   F:text;
7. begin
8.   System.Assign(F,Logfilename);
9.   if FileExists(LogFileName) then
10.      System.append(F)
11.   else
12.     System.Rewrite(F);
13.   System.writeln(S);
14.   system.close(f);
15. end;

From memory, will test it later....

[cdbc]

Hi Thaddy
He's reading streamed output from TProcess and logging it to a filestream...
His code is allright. His extra use of TMemoryStream doesn't hurt anybody.
Regards Benny

[Thaddy]

Except e.g: TMemoryStream --> TStringStream and the buffer --> a buffer of AnsiChar, not byte.
Again I find his approach cumbersome.

[ALLIGATOR]

Could it be something like this?

The point is to break it down into smaller functions with clear names that describe what they do

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

1. function WriteStreamToFile(AFileName: String; AStream: TStream): Boolean;
2. begin
3.   //Create a FileStream to save (append) to the LOG
4.   LogFName:= ExtractFilePath(ParamStr(0)) + 'somefile.log';
5.   if FileExists(LogFName) then begin
6.     aFs:= TFileStream.Create(LogFName, fmOpenReadWrite or fmShareExclusive);
7.     aFs.Seek(0, soFromEnd); //Move to the end of the file
8.   end else
9.     aFs:= TFileStream.Create(LogFName, fmCreate or fmShareExclusive);
10.   end;
11.  
12.   aFs.CopyFrom(AStream, AStream.Size);
13.   aFs.WriteBuffer(Pointer(LineEnding)^, Length(LineEnding));
14.   aFs.Free;
15. end;
16.  
17. function SaveTheFunnyLog:string;
18. var
19.   Buffer: array[0..2048 - 1] of byte;
20. ...
21. ...
22.   aProc:= TProcess.Create(nil);
23.   aProc.Options:= [poUsePipes];
24.   aProc.Execute;
25. ...
26. ...
27.   OutputStream:= TMemoryStream.Create;
28.   repeat
29.     BytesRead:= aProc.Output.Read(Buffer, BUF_SIZE);
30.     OutputStream.Write(Buffer, BytesRead)
31.   until BytesRead = 0;  // Stop if no more data is available
32.  
33.   aProc.Free;
34.  
35.   OutputStream.Position:= 0; //Make sure all data is copied from the start
36.   WriteStreamToFile('somefile.log', OutputStream);
37.   OutputStream.Free;
38.  
39.   S:= '';
40.   SetLength(S, Length(Buffer));
41.   Move(Buffer[0], S[1], Length(Buffer));
42.   Result:= S;
43. end;