The professional review

Review behavior before formatting

A review is a deliberate attempt to find what the design has overlooked. It is not a ceremony in which a second person confirms that the indentation looks tidy. Begin with the machine brief, behavior contract, and test evidence. Then inspect whether the code implements that agreement.

Use a small model filling station for this review. A bottle enters, a valve fills it, and the bottle leaves. The programmer has submitted a short routine that appears to work in a normal animation:

IF Start THEN
    Run := TRUE;
END_IF;

IF Stop THEN
    Run := FALSE;
END_IF;

IF Run AND BottlePresent THEN
    FillTimer(IN := TRUE, PT := FillTime);
    ValveOpen := TRUE;
END_IF;

IF FillTimer.Q THEN
    ValveOpen := FALSE;
    GoodCount := GoodCount + 1;
END_IF;

IF Reset THEN
    Run := TRUE;
END_IF;

Do not fix it line by line yet. First ask what each variable means and which assumptions make the happy animation succeed.

Build findings that someone can act on

Each finding needs a trigger, observed or predicted behavior, consequence, and correction direction. “Bad timer code” is not enough. “After the timer reaches Q, GoodCount increments on every evaluation while Q remains true, so one bottle produces many counts” is a testable finding.

Here are the important defects in this model submission:

Some runtime details depend on the selected timer implementation, which is another reason to require its documentation. The conditional invocation is a review concern even before deciding exactly how a particular target retains elapsed state.

Repair the ownership model first

The code has no explicit concept of an accepted bottle. BottlePresent directly causes filling whenever Run is true. There is no separation between waiting, filling, completing, and waiting for departure.

Replace that ambiguity with states: WaitingForBottle, Filling, AwaitingDeparture, and Interrupted. Accept a bottle only when the station is enabled, a new bottle is present under the entry rules, and the recipe is valid. Capture its identity or cycle number and the active fill duration. Count completion once on the transition out of Filling. Remain in AwaitingDeparture until the bottle leaves before accepting another.

Give the valve request one owner and assign it every evaluation. In the model, it is true only while Filling and ordinary production permission remains valid. The physical valve can still fail, which belongs in plant feedback and fault tests rather than being hidden by the output variable name.

Show a corrected design fragment

The following fragment illustrates the improved decision structure. Command edges, recipe validation, initialization, and target-specific declarations belong to the surrounding model contract.

FillTimer(IN := State = FILLING, PT := ActiveFillTime);
CompletedThisScan := FALSE;

IF (State = FILLING) AND
   (StopRequest OR NOT ProductionPermission OR NOT BottlePresent) THEN
    State := INTERRUPTED;
    FaultReason := FILL_INTERRUPTED;
ELSE
    CASE State OF
        WAITING_FOR_BOTTLE:
            IF StartPulse AND BottlePresent AND RecipeValid
               AND ProductionPermission AND NOT StopRequest THEN
                ActiveFillTime := RequestedFillTime;
                State := FILLING;
            END_IF;
        FILLING:
            IF FillTimer.Q THEN
                CompletedThisScan := TRUE;
                State := AWAITING_DEPARTURE;
            END_IF;
        AWAITING_DEPARTURE:
            IF NOT BottlePresent THEN
                State := WAITING_FOR_BOTTLE;
            END_IF;
        INTERRUPTED:
            IF ResetPulse AND RecoveryConfirmed THEN
                State := WAITING_FOR_BOTTLE;
            END_IF;
    ELSE
        FaultReason := INVALID_STATE;
        State := INTERRUPTED;
    END_CASE;
END_IF;

ValveRequest := (State = FILLING)
                AND ProductionPermission AND NOT StopRequest;
IF CompletedThisScan THEN
    CompletedFillCount := CompletedFillCount + 1;
END_IF;

This model requires a fresh start for each bottle. An automatic feeder would use an explicit accepted-part event instead; do not leave that difference implicit. Timer activation begins on the evaluation after entry to Filling, so the specified timing origin must include that choice or use an acceptance timestamp.

CompletedFillCount is deliberately not called GoodCount. A timed fill does not prove product quality. Quality release may require weight, inspection, or another process criterion. Names should not claim evidence the program never obtains.

Review the evidence as seriously as the code

Ask for tests covering stop during filling, disappearing bottle, held Start, held Reset, recipe edits, invalid state, repeated cycles, and a stuck physical valve in the model. Verify the count increments once and that completion is not recorded for an interrupted fill.

Ask what is retained across restart. Retaining a count may be sensible; retaining Filling without a recovery policy may not be. Review task ownership and whether another routine writes the valve request. A locally correct function cannot guarantee global single ownership if its output is overwritten elsewhere.

Check the operator explanation. “Fault 7” is technically possible and operationally weak. The diagnostic should identify the interrupted action, relevant feedback, and the approved next step without implying that acknowledgement resolves the physical cause.

Apply standards with evidence

Use the project's coding conventions to check naming, structure, comments, and maintainability. PLCopen publishes construction guidance that can inform those conventions; it does not replace the project specification or confer compliance on a reviewed program. PLCopen Software Construction Guidelines.

For safety-related boundaries, verify that the ordinary program interfaces match the assigned safety requirements and that responsibility is identified. The code reviewer should not infer a safety integrity claim from a variable named SafetyOK. Keep the distinction developed in safety and standards.

Try it

A reviewer asks for comments on every line of the corrected fragment but does not request interruption tests. Another reviewer asks for the active-recipe rule, output cross-reference, and evidence for count accuracy. Which review is more likely to find an expensive defect? Write one useful comment and one useful test assertion.

Work through the answer

The second review targets behavior and assumptions. Comments are useful when they preserve reasoning, but repeating State := FILLING as “set state to filling” adds little.

A useful comment explains a choice: “Capture the accepted duration so recipe edits apply only to later bottles.” A useful assertion is: “For each accepted cycle, CompletedFillCount increases by exactly one only after uninterrupted completion, regardless of how long the bottle remains present afterward.”

Run that assertion over several durations and departure delays. Then interrupt the fill and verify no increment. Review is strongest when a concern becomes a repeatable test and a clear design rule, not merely a stylistic preference.