Replace all occurrence of REP [block] with value of the block

I have a block with an undefined data and a pair of REP [code]:

arr: [
    REP ["code1"]
    | 'something
     "f"
    | [ f | 3 ]
    REP [40 + 2]
    ]

The pair can be everywhere in the arr.

I replace every occurrence of a pair REP [code] with a value from evaluating the code.

t: true

while [t == true] [
    f: find arr 'REP
    either f [
        t: true
        change/part f do first next f 2
        ][
        t: false
        ]
]

Does this code follow common best practices? Does it have some security issues? Is there any problems with a performance using this code?

Does it have some security issues?

Well, you're doing an evaluation of code. If that code comes from a foreign source somehow, that could be a problem. If it's your code, it's no more dangerous than anything else.

But as you wrote it, it does have the property that it can infinite loop within itself. Because after your substitution you start at the beginning again:

arr: [REP [quote REP]]


That's a stable state, but you could keep growing without bound too.

My answers will assume you didn't want this.


Does this code follow common best practices?

It's C-like. Rebol is a great language for writing bad C code. :-)

Using COMPOSE is the obvious answer here:

>> compose [
("code1")
| 'something
"f"
| [ f | 3 ]
(40 + 2)
]
== ["code1" | 'something "f" | [ f | 3 ] 42]


And if you're worried your content contains parens that you want to leave in place, you can quote them:

>> compose [1 (quote (2 3)) 4]
== [1 (2 3) 4]


But if you want to work with the input as written, there's a lot of approaches.

The most "sensible" way--that the wiki regarding features in R3-Alpha suggests would work (but doesn't seem to)--would be:

>> parse arr [
any [
change ['REP blk: block!] (do blk/1)
|
skip
]
]


Which would more or less capture the essence of the task. Any number of times, try and match a pattern of REP followed by a BLOCK!, marking the position prior to the block. Then substitute the execution of do blk/1 for that code". Otherwise skip and keep looking.

It gets almost there but misses the execution. So it splices in do blk/1 literally:

== [do blk/1 | 'something "f" | [f | 3] do blk/1]


Try reporting a bug to...whoever works on that. Or maybe to Red. :-)

I've seen you mention you don't like UNTIL, so here's a FORALL-based solution...

forall arr [
if arr/1 = 'REP [
remove arr
arr: back change arr do arr/1
]
]


(Note: I think forall should be called FOR-NEXT, because all it does is advance the argument until the end via NEXT. You can modify it during the loop, and then it will run NEXT again. It will be reset at the end.)

As a talking point on your code: Note that there are two FALSE? values (NONE! and LOGIC! false), and everything else is TRUE? (except unset which is neither). So it's not that common to work with literal true and false. NONE is almost always better as a contrast to some value you are interested in.

Just to show what kinds of tricks are available in an "imperative-series-assembly-language" with a WHILE approach: if we are walking along the array replacing, we might use the position back from FIND not being NONE! to cue continuation:

head while [arr: find arr 'REP] [
arr: change/part arr do arr/2 2
]


It's able to work because change is designed for chaining, so it returns the point after the replacement. Also because WHILE returns the last evaluation of the body. So even though the condition wipes out the value of arr when the find comes back with NONE!, it can be recovered because the last time the body evaluated it was the end position of the CHANGE...and you can seek back to the head from that.

So there are a few possibilities.

StackExchange Code Review Q#115011, answer score: 7