#1 08 Sep 2019 10:12

Rekrul
Member
Registered: 17 Apr 2016
Posts: 49

ARGH!!! Someone please shoot me now and put me out of my misery!!!

The more I try to write complex scripts, the more I have a burning hatred for whoever designed this broken mess.

I have this code in my script;

if !width! gtr 640 set width=1280
if !width! gtr 540 (if !width! leq 640 set width=640)
if !width! gtr 500 (if !width! leq 540 set width=540)
if !width! gtr 400 (if !width! leq 500 set width=500)
if !width! gtr 250 (if !width! leq 400 set width=400)
if !width! leq 250 set width=250

This works, but I wanted to speed it up as much as possible by making it only try the rest of the comparisons if the ones before them had failed. So I changed it to;

if !width! gtr 640 set width=1280 else (
if !width! gtr 540 (if !width! leq 640 set width=640) else (
if !width! gtr 500 (if !width! leq 540 set width=540) else (
if !width! gtr 400 (if !width! leq 500 set width=500) else (
if !width! gtr 250 (if !width! leq 400 set width=400) else (
if !width! leq 250 set width=250)))))

This causes it to generate the error "Maximum setlocal recursion level reached." Not only that, but the rest of the script, which is supposed to rename files, fails to trigger. It doesn't even work before it hits the recursion error. It should be doing something, even if it renamed the files to the wrong thing, but it doesn't. To be fair, it properly renames ONE file, but I have no idea why.

For reference, I enable Delayed Expansion inside a FOR loop after the filename variables have been defined, then I ENDLOCAL before the start of the next loop. If I don't define the variables outside of Delayed Expansion, it removes "!" from the filenames. If I don't use Delayed Expansion for everything else, nothing works. This has never caused a problem before since it's reset for every loop and there's never a condition where it would start a new loop without ENDLOCAL. Well, until now apparently. roll

The above bit of code is the ONLY thing I changed, and I made sure that there were no unmatched parentheses, and that it didn't improperly jump out of any loops.

The first example works, no matter how many files it's run on. The second fails. Why???

What arcane rule have I violated now?

Last edited by Rekrul (08 Sep 2019 10:44)

Offline

#2 08 Sep 2019 17:31

Aacini
Member
Registered: 05 Dec 2012
Posts: 148

Re: ARGH!!! Someone please shoot me now and put me out of my misery!!!

Ok. Lets review a simple example:

if !width! gtr 640 set width=1280 and any string you want...

It should be obvious that the entire string is assigned to width variable. Why? Because there is not any delimiter in the string. In this case:

if !width! gtr 640 set width=1280 & echo any string you want...

The string assigned is "1280 " because the & is a delimiter.

if !width! gtr 640 set width=1280 else (

Should I explain previous case?  Hint: the " else" string is not a delimiter...

Antonio

Offline

#3 09 Sep 2019 04:57

Rekrul
Member
Registered: 17 Apr 2016
Posts: 49

Re: ARGH!!! Someone please shoot me now and put me out of my misery!!!

Aacini wrote:

It should be obvious that the entire string is assigned to width variable. Why? Because there is not any delimiter in the string. In this case:

Thanks. You're right, I didn't think of that. I still get confused by the fact that spaces only act as delmiters in some situations (like on the command line).

Aacini wrote:
if !width! gtr 640 set width=1280 & echo any string you want...

I have rather spotty luck using "&" to chain commands. Sometimes it works fine and sometimes it seems to just ignore it. I ended up solving the problem with;

if !width! gtr 640 (set width=1280) else (
if !width! gtr 540 (if !width! leq 640 set width=640) else (
if !width! gtr 500 (if !width! leq 540 set width=540) else (
if !width! gtr 400 (if !width! leq 500 set width=500) else (
if !width! gtr 250 (if !width! leq 400 set width=400) else (
if !width! leq 250 set width=250)))))

I just get so frustrated with the oddities of batch scripting that I often overlook simple mistakes. In my defense, there are times when things simply don't seem to work right. The other day while trying to figure out why something was misbehaving, I created a test script to test just the part I was having a problem with and I kept paring it down until it was at the absolute minimum. I didn't keep the actual script file, but I ended up with;

set done=1

setlocal enabledelayedexpansion

if "!done!"=="1" echo success

It works now but I SWEAR I had this exact same code and it didn't print anything until I added "NOT" after the IF. I looked at it for at least 20 minutes, checking for invisible characters or extra spaces that might have confused it, but it simply didn't work. I was going to make a post about it, but then I went on to something else, had other problems, etc. It was like I had crossed over into opposite land. I even added a line to print the value of "done" and it printed "1" every time. roll

Offline

#4 09 Sep 2019 06:59

Aacini
Member
Registered: 05 Dec 2012
Posts: 148

Re: ARGH!!! Someone please shoot me now and put me out of my misery!!!

Well, in this segment of code:

if !width! gtr 640 (set width=1280) else (
if !width! gtr 540 (if !width! leq 640 set width=640) else (
if !width! gtr 500 (if !width! leq 540 set width=540) else (
if !width! gtr 400 (if !width! leq 500 set width=500) else (
if !width! gtr 250 (if !width! leq 400 set width=400) else (
if !width! leq 250 set width=250)))))

You have redundant tests. Let's review they.

In the first IF, the else part is executed when !width! is NOT gtr 640; this means that in this case !width! is leq 640, right? In this way, the if !width! leq 640 command in the second line is entirely superfluous. The same apply for the rest of nested IF's...

You need to rearrange the logic to eliminate redundant cases AND to simplify the nesting. For example:

       if !width! gtr 640 (set width=1280
) else if !width! gtr 540 (set width=640
) else if !width! gtr 500 (set width=540
) else if !width! gtr 400 (set width=500
) else if !width! gtr 250 (set width=400
) else                    (set width=250
)

This construct is commonly know as "switch" or "case".

Another, simpler way to achieve the same result, is this:

set prev=1280
for %%a in (640 540 500 400 250 0) do (
   if !width! gtr %%a if !width! leq !prev! set width=!prev!
   set prev=%%a
)

A more interesting simplification could be achieved via a long arithmetic expression. For example:

@echo off
setlocal EnableDelayedExpansion

set "limits=0 250 400 500 540 640 1280"

:loop
set /P "width=Width: "
if errorlevel 1 goto :EOF

set /A "token=1-(((%limits: =-width)>>31)+((%-width)>>31))"
for /F "tokens=%token%" %%a in ("%limits%") do set index=%%a
echo %index%
echo/

goto loop

Enjoy it! lol

Antonio

Offline

#5 09 Sep 2019 12:09

Rekrul
Member
Registered: 17 Apr 2016
Posts: 49

Re: ARGH!!! Someone please shoot me now and put me out of my misery!!!

Aacini wrote:

You have redundant tests. Let's review they.

In the first IF, the else part is executed when !width! is NOT gtr 640; this means that in this case !width! is leq 640, right? In this way, the if !width! leq 640 command in the second line is entirely superfluous. The same apply for the rest of nested IF's...

You're right again. The two tests in each line were necessary when each was a separate command because it was going through all of them regardless. When I nested it, I simply added the ELSE commands to what I already had.

Aacini wrote:

Another, simpler way to achieve the same result, is this:

I think your first example is best. The whole reason I nested them in the first place rather than having separate statements was to try and speed up the execution. I didn't want it to run through all six possibilities if it didn't have to. I realize that the difference is probably only milliseconds, but by the time I've accumulated enough files to fill a DVD, it can range in the several thousands and if I'm going to run the script on the whole collection, I want to squeeze every little bit of speed I can out of it. I got a pretty decent speed boost by changing it to only check the width of images that fit a particular pattern, rather than all of them as I had done previously.

Aacini wrote:

A more interesting simplification could be achieved via a long arithmetic expression. For example:

Arithmetic expressions were never my strong suit, and it uses a bunch of conventions I'm not familiar with, so I basically understood none of that. sad

To be perfectly honest, I've never been a particularly good programmer. The only languages I ever really learned were BASIC and a little assembly on the C64. Which is why I started getting into batch scripts in the first place. The commands are enough like BASIC that it mostly makes sense to me when Delayed Expansion isn't screwing things up. Part of my desire is to write scripts that do useful stuff, but part of it is also to create something, even if it's a script that only myself and maybe a friend or two will use.

BTW, the full script is posted here if you're curious;

https://ss64.org/viewtopic.php?id=2374

I've revised it since then and figured out how to add working counters to it. After a little more testing, I'm going to post the whole thing in that thread just to kind of bookend my struggles with it. smile

Last edited by Rekrul (09 Sep 2019 12:11)

Offline

#6 09 Sep 2019 14:12

bluesxman
Member
From: UK
Registered: 29 Dec 2006
Posts: 1,101

Re: ARGH!!! Someone please shoot me now and put me out of my misery!!!

This seems to be easier to parse mentally (though I've not tested it)

set "w=1280"
for %%a in (640 540 500 400 250) do if !width! LEQ %%a set "w=%%a"
set /a width=w

EDIT: sorry just after I submitted this, I realised it was similar to one of aacini's suggestions, but I wrote it in isolation, honest!

EDIT2: Actually this is more like what you're after I think?

set "w=1280"
for %%a in (640 540 500 400 250) do (
  if !width! LEQ %%a (set "w=%%a") ELSE (goto :break_for)
)
:break_for
set /a width=w

Last edited by bluesxman (10 Sep 2019 08:42)


cmd | *sh | Ruby | Chef

Offline

#7 15 Sep 2019 10:08

Rekrul
Member
Registered: 17 Apr 2016
Posts: 49

Re: ARGH!!! Someone please shoot me now and put me out of my misery!!!

bluesxman wrote:

This seems to be easier to parse mentally (though I've not tested it)

That seems like it would work, although it has the same problem as my original solution, namely that it runs through all the possibilities each time.

bluesxman wrote:

EDIT2: Actually this is more like what you're after I think?

I originally tried using a GOTO to skip the rest of the comparisons. I used an additional variable as a flag and then added a test for that flag. However, GOTO apparently has a bug in that it breaks parenthesis. Since this whole section is inside a loop, it breaks the loop.

So far, I've used Aacini's first example of nesting the IF/ELSE commands and just eliminating the second test for each one, as the previous command already covers that.

Offline

#8 18 Sep 2019 11:00

bluesxman
Member
From: UK
Registered: 29 Dec 2006
Posts: 1,101

Re: ARGH!!! Someone please shoot me now and put me out of my misery!!!

Ah yes a double nested loop would fail.  An alternative may be to do something like this:

for %%s in (your outer loop foo bar) do (
  call :subroutine
)

goto :EOF

:subroutine
set "w=1280"
for %%a in (640 540 500 400 250) do (
  if %width% LEQ %%a (set "w=%%a") ELSE (goto :break_for)
)
:break_for
set /a width=w
goto :EOF

But you will experience a small time penalty for the :call (which may become noticable if the outer loop processes a lot of items).


cmd | *sh | Ruby | Chef

Offline

Board footer

Powered by FluxBB