How to refactor a code with lot of check ?

[Golfy]

Here is one of my procedures. The job is to read a serial bus for message : a message start with 0F and end with 04. Length is variable (but indicated at fourth byte).

Code: Select all

Procedure.i RecordFrame(ClientID, List Frame.VMBFrame(), *BNin)
	; This procedure read the network buffer, extract Velbus frame and save them into LinkedList (with 'VMBFrame' structure)
	; ClientID      = TCP Connexion ID
	; proc.Return   = New frames number
	; USAGE : 
	;    Global NewList msg.VMBFrame()
	;    cpt = RecordFrame(ClientID, msg(), *BNin)
	;
	LastElement(Frame())
	resultat.s =""
	If *BNin 
		If NetworkClientEvent(ClientID) = #PB_NetworkEvent_Data
			RXLen = ReceiveNetworkData(ClientID, *BNin, 4096)
			;Debug "-> "+readmsg(*BNin,RXLen)+"  ("+Str(RXLen)+")"
			t=0
			FrameNumber = 0
			Repeat
				;Debug t
				If PeekA(*BNin+t) = $0F																			; search for a START $0F
					fixlen = 4																								; base len : 4 bytes (0F FX @@ Size .. .. .. ## 04)
					If t+fixlen < RXLen																				; VERIFY if still in the buffer (no overflow)
						varlen = PeekA(*BNin+t+3) & $0F													; Size byte value is just 4 bits
						crcloc = t+fixlen+varlen																; CheckSum position is T actual position + 4 + Size
						fintrame = crcloc+1																			; Try to find end of frame :  CheckSum + 1
						RelativeLen = fixlen+varlen+2														; [0F][FX][@@][Size][Fct][][]...[##][04] = Fixlen+Size+1+1 (include byte at pos. 0)
						If fintrame <= RXLen																		; VERIFY if end of frame still in the buffer (no overflow again)
							If PeekA(*BNin+fintrame) = $04												; Check if a END is at the right location
								; == GOOD MESSAGE ==
								;Debug "GOOD : "+ReadMsg(*BNin,fintrame+1)
								frameNumber + 1
								AddElement(Frame())
								Frame()\long.i = RelativeLen
								Frame()\BFrame = AllocateMemory(RelativeLen)
								Frame()\BMask = AllocateMemory(RelativeLen)
								Frame()\Stamp.i = Date()
								CopyMemory(*BNin+t, Frame()\BFrame, RelativeLen)		; Start is byte 0. Example: for z = 0 to RelativeLen-1 !!!
								t = fintrame																				; add the offset to T (don't try to search a frame in this good message)

								If PeekA(Frame()\BFrame+4) = 0
									buttonNb = PeekA(Frame()\BFrame+5) | PeekA(Frame()\BFrame+6) | PeekA(Frame()\BFrame+7)
									Debug "Message de $"+Hex(PeekA(Frame()\BFrame+2))+" fonction "+Hex(PeekA(Frame()\BFrame+4))+" Number : "+RSet(Bin(buttonNb,#PB_Ascii),8,"0")
								EndIf

							Else
							  Debug "Frame len error"
							EndIf
						EndIf	
					Else																											; No enough space in buffer for a new frame... stop the loop
						t = RXLen
					EndIf
				EndIf
				t = t + 1
			Until t => RXLen
		EndIf
	EndIf
	ProcedureReturn FrameNumber.i
EndProcedure

[netmaestro]

I confess that after five solid minutes of scanning your post I have no idea what it is you're asking. Any chance of elaborating on the goal?

[Thunder93]

It was suggested on another topic for this person to refactor / improve his coding and avoid deeply nested If statements because of visual difficulty for maintenance.

[Thunder93]

I see nothing wrong with that procedure, looks good. Unless portion of that procedure is being called multiple times from different points, I wouldn't try splitting.

[Shield]

Since the scope of this code is unknown and because it is not a runable example, I'll give some general tips:

1. First, define your variables and use EnableExplicit.
2. If *BNin could directly end the procedure and doesn't need to be wrapped around everything.
3. You seem to use a lot of debug code. For example, what is the inner most If statement doing?
4. Debug code should not remain in the final executable, so using CompilerIf would be better here.
5. buttonNb isn't used anywhere other than in the debug statement. If it's a global variable, you should change that.
6. The calculation within the Repeat statement could be put in another procedure.

[Thunder93]

Hi Shield.

Regarding #4. Leaving Debug lines should be okay? I thought Debug lines are ignored / not included .. when creating executable.

[Tenaja]

Hi Shield.

Regarding #4. Leaving Debug lines should be okay? I thought Debug lines are ignored / not included .. when creating executable.

They are, but the "else" command is still compiled, even if it is empty. Unless there are some fancy optimizations (and PB has very few) the else is jumped over after the IF condition is completed, so the jump is to the very next instruction...a waste of time.

[Thunder93]

Tenaja, thanks for responding. I understand what is being said now, thanks.

[mhs]

A slightly different interpretation of many others. The code is the same, only slightly restructured.

Code: Select all

Procedure.i RecordFrame(ClientID, List Frame.VMBFrame(), *BNin)

   ; This procedure read the network buffer, extract Velbus frame and save them into LinkedList (with 'VMBFrame' structure)
   ; ClientID      = TCP Connexion ID
   ; proc.Return   = New frames number
   ; USAGE :
   ;    Global NewList msg.VMBFrame()
   ;    cpt = RecordFrame(ClientID, msg(), *BNin)
   ;
   
  Define.s resultat
  Define.i t, FrameNumber, ...

  LastElement(Frame())

  If Not *BNin
    ProcedureReturn 0
  EndIf
  
  If NetworkClientEvent(ClientID) <> #PB_NetworkEvent_Data
    ProcedureReturn 0
  EndIf

  RXLen = ReceiveNetworkData(ClientID, *BNin, 4096)
  ;Debug "-> "+readmsg(*BNin,RXLen)+"  ("+Str(RXLen)+")"
  t=0
  FrameNumber = 0

  Repeat

    ;Debug t
    
    If PeekA(*BNin + t) <> $0F                                                 ; search for a START $0F
      t = t + 1
      Continue
    EndIf

    fixlen = 4                                                                 ; base len : 4 bytes (0F FX @@ Size .. .. .. ## 04)

    If t + fixlen >= RXLen                                                     ; VERIFY if still in the buffer (no overflow)
      t = RXLen + 1                                                            ; No enough space in buffer for a new frame... stop the loop
      Continue
    EndIf

    varlen      = PeekA(*BNin + t + 3) & $0F                                   ; Size byte value is just 4 bits
    crcloc      = t + fixlen + varlen                                          ; CheckSum position is T actual position + 4 + Size
    fintrame    = crcloc + 1                                                   ; Try to find end of frame :  CheckSum + 1
    RelativeLen = fixlen + varlen + 2                                          ; [0F][FX][@@][Size][Fct][][]...[##][04] = Fixlen+Size+1+1 (include byte at pos. 0)

    If fintrame <= RXLen                                                       ; VERIFY if end of frame still in the buffer (no overflow again)
      t = t + 1
      Continue
    EndIf
    
    If PeekA(*BNin+fintrame) = $04                                             ; Check if a END is at the right location
    
      ; == GOOD MESSAGE ==
      ;Debug "GOOD : "+ReadMsg(*BNin,fintrame+1)
      frameNumber + 1
      
      AddElement(Frame())
      Frame()\long.i  = RelativeLen
      Frame()\BFrame  = AllocateMemory(RelativeLen)
      Frame()\BMask   = AllocateMemory(RelativeLen)
      Frame()\Stamp.i = Date()
      
      CopyMemory(*BNin+t, Frame()\BFrame, RelativeLen)                         ; Start is byte 0. Example: for z = 0 to RelativeLen-1 !!!
      t = fintrame                                                             ; add the offset to T (don't try to search a frame in this good message)
    
      If PeekA(Frame()\BFrame+4) = 0
        buttonNb = PeekA(Frame()\BFrame+5) | PeekA(Frame()\BFrame+6) | PeekA(Frame()\BFrame+7)
        Debug "Message de $"+Hex(PeekA(Frame()\BFrame+2))+" fonction "+Hex(PeekA(Frame()\BFrame+4))+" Number : "+RSet(Bin(buttonNb,#PB_Ascii),8,"0")
      EndIf
    
    Else
      Debug "Frame len error"
    EndIf

    t = t + 1

  Until t => RXLen

  ProcedureReturn FrameNumber.i

EndProcedure

[Tenaja]

Many coders on this forum use only two spaces for indentation. I find, especially with deeply nested constructs, that four spaces makes it easier to see the indentation level. It helps to have a wide monitor, which is now common. Any time I paste forum code into the IDE, the first thing I do is select-all and hit ctrl-L to change the indentation to my preference. I guess I just don't see the point in indenting if you do it so slight that you cannot visually align indents at the top and bottom of the screen.

[Shield]

It was quite funny for me to realize that I also exclusively used two spaces when I was only using PB. Basically, as soon as I touched another language (C++ in my case) I switched to 4 space tabulators.

[Golfy]

Thanks to all for adjusting my words (I'm french with a bad english but everybody knows that french aren't good in other language ).
Back to the code, I will try to have 3 or 4 characters tabulation.
Yes, I've to remove 'else' just for debug and remove NbButton... but I know that debug instructions are removed when creating a executable code (out of the purebasic editor).

This procedure illustrate the problem of increment larger than 2 pages (on one screen).

Last : I know I should use EnableExplicit... but I'm lazzy in procedure (as I know all variables are destructed at the end of procedure

[Tenaja]

Last : I know I should use EnableExplicit... but I'm lazzy in procedure (as I know all variables are destructed at the end of procedure

I think you have missed the point, which is not about releasing the local variables. The primary point is to avoid errors caused by typos...for instance, if you do something like this...

xposition = something
...
xpositoin = somethingelse ; note typo in variable name

with EnableExplicit, the typo is caught as a syntax error. Without it, there is no error, just a bug--which may be difficult to find.

In addition, let's say you have a global variable; the same applies--rather than thinking you have referenced the global, without EnableExplicit the compiler sees you have defined a new local variable. It is much better to stop at the typo.

Sometimes doing things the lazy way is the far slower way. On the other hand, maybe you never have typos so you do not have this issue, but many of us do...