nested IF statements, best practice (newbie question)

[AVaughn]

Hello everyone,

I have occasionally stumbled upon this forum, there is lots of brilliance here. I am not very skillful with batch, but I have some coding experience in other languages.

I have a very basic question today - it's about best practice, professionalism, practicality.

Here is a snippet of code from my script, this section of code checks for some required .dll files that an .exe will need. These checks occur before attempting to run the .exe, because if they're not detected, you get a nasty "loud" popup message outside of cmd.

Code: Select all

IF EXIST libiconv2.dll (
	set 3_DEP=y
	echo libiconv2.dll exists
) else (
	set error=6
	goto errorexit
)

IF EXIST libintl3.dll (
	set 4_DEP=y
	echo libintl3.dll exists
) else (
	set error=7
	goto errorexit
)

IF EXIST libcharset1.dll (
	set 5_DEP=y
	echo libcharset1 exists
) else (
	set 5_DEP=n
	echo optional dependency libcharset1.dll missing

This is just some testing, for example all of the echoed messages are for troubleshooting. To me, this seems very "primitive", I am wondering if there is any better way to write this? I have made an attempt to code something using "nested" if statements, it looks much nicer, and allows any reader of the script to immediately see that these EXIST checks are related to each other, however it doesn't work at all. Here is my attempt:

Code: Select all

IF EXIST libiconv2.dll (
	set 3_DEP=y
	echo libiconv2.dll exists
	IF EXIST libintl3.dll (
		set 4_DEP=y
		echo libintl3.dll exists
		IF EXIST libcharset1.dll (
			set 5_DEP=y
			echo libcharset1 exists
		) else (
			set 5_DEP=n
			echo optional dependency libcharset1.dll missing
	) else (
		set error=7
		goto errorexit
) else (
	set error=6
	goto errorexit
)

So the question is: is it worth trying to make this 2nd method work, can it work? If it can, where did I go wrong? I apologise in advance as this is probably very basic question, unfortunately I could not find many good examples of nested IF statements like this...

[Aacini]

IMHO the first code is enough for this purpose. Of course, there is code that requires a deep nesting of commands/parentheses, but in this case I think that a deep nesting of IF commands makes the code unnecesarily complicated.

Anyway, your second code don't works because two right parentheses are missing:

Code: Select all

IF EXIST libiconv2.dll (
	set 3_DEP=y
	echo libiconv2.dll exists
	IF EXIST libintl3.dll (
		set 4_DEP=y
		echo libintl3.dll exists
		IF EXIST libcharset1.dll (
			set 5_DEP=y
			echo libcharset1 exists
		) else (
			set 5_DEP=n
			echo optional dependency libcharset1.dll missing
		)   <- This "endif" is missing
	) else (
		set error=7
		goto errorexit
	)   <- This "endif" is missing
) else (
	set error=6
	goto errorexit
)

A couple recommendations:

- The important one first: it is a bad idea to use variables that start in digit, like 3_DEP, 4_DEP or 6_DEP. I strongly encourage you to change the first char by a letter.

- I personally used to use 3 spaces for the left margin of nested structures (commands). A TAB character implies 8 spaces, that is too much for deep nesting. For example:

Code: Select all

IF EXIST libiconv2.dll (
   set 3_DEP=y
   echo libiconv2.dll exists
   IF EXIST libintl3.dll (
      set 4_DEP=y
      echo libintl3.dll exists
      IF EXIST libcharset1.dll (
         set 5_DEP=y
         echo libcharset1 exists
      ) else (
         set 5_DEP=n
         echo optional dependency libcharset1.dll missing
      )   <- This "endif" is missing
   ) else (
      set error=7
      goto errorexit
   )   <- This "endif" is missing
) else (
   set error=6
   goto errorexit
)

Antonio

[OJBakker]

Here is a different approach using a for-loop.

Code: Select all

rem with the echo messages	
set "error="
for %%D in ("libiconv2.dll", "libintl3.dll" "libcharset1.dll") do (
   if exist "%%D" (
      set "%%~nd=y"
      echo %%D exsists
   )
)
if not defined libiconv2 (set "Dep_3=y" & set error=6)	  
if not defined libintl3 (set "Dep_4=y" & set error=7)
if not defined libcharset1 (set "Dep_5=n" & (echo optional dependency libcharset.dll missing))
; cleanup variables
for %%D in ("libiconv2.dll", "libintl3.dll" "libcharset1.dll") do set "%%~nd="

if defined error goto errorexit

Code: Select all

rem without the echo messages
set "error="
for %%D in ("libiconv2.dll", "libintl3.dll" "libcharset1.dll") do if exist "%%D" set "%%~nd=y"

if not defined libiconv2 (set "Dep_3=y" & set error=6)	  
if not defined libintl3 (set "Dep_4=y" & set error=7)
if not defined libcharset1 set "Dep_5=n"
; cleanup variables
for %%D in ("libiconv2.dll", "libintl3.dll" "libcharset1.dll") do set "%%~nd="

if defined error goto errorexit

[miskox]

One of the options is (because looks like you don't have filenames changing all the time):

Code: Select all

set files_exist=0
if exist libiconv2.dll if exist libintl3.dll if exist libcharset1.dll set files_exist=1

and then you just check for 'files_exist' variable. You say

Code: Select all

all of the echoed messages are for troubleshooting.

so maybe you don't have to write exactly which .dll is missing. If you need this info then different approach is needed. Maybe something like setting the 'bit' for each file (file 1 has a value of 1, file 2 has a value of 2, file 3 has a value of 4...) and you add these values if file is missing.


Saso

[AVaughn]

Antonio, thanks for your reply.

Firstly, you helped correct my failed attempt at nesting, that is really nice of you - secondly, you reminded me that sometimes extra complexity is unnecessary. I can't think of a functional difference between the nested and not nested versions, perhaps I should focus more on functionality than on how pretty code looks, code which won't even be seen by end users.

If I ever need nested IF statements, I now know how to do it, which may help in the future. Also, I will take your warning about variable names into account!

OJBakker, thanks for offering an alternate suggestion, it's definitely shorter in terms of lines of code, I think, if I ever had a large number of files to check, where each one needs it's own variables/message, your suggestion would be quite an efficient one. I noticed a small error where if a file is not found, %error% is set, and also the %DEP_X% is set to "y", these two are supposed to be mutually exclusive, error is set if file is not found, dep_n is set to "y" if it is found.

I like the idea of checking multiple things, then having one goto at the end, like this:

Code: Select all

if defined error goto errorexit

I will find ways to use this, it might reduce the number of lines slightly.

Saso, I did indeed come across examples such as yours, in the format of IF X IF Y IF Z DO A. Realistically, your approach may be the best due to being the simplest, if any files are missing: an error is thrown, and a list of required files is shown to the user - which ones are there, which ones aren't, this can be for the end user to figure out. Maybe it is too ridiculous for the program to know which files are missing - but when I came up with the idea, I thought it would be more impressive, and more useful to the user.

[miskox]

After some thinking: the long IF will not work correctly. It will work if *all* files are missing and not if there is at least one file present. Maybe you could use this:

Code: Select all

@echo off
REM create temporary files
break>a.a
break>b.b
break>c.c

REM list files here:
set file_list=a.a b.b&rem
REM if there are many files use this to add more (so the line above is not too long (to view on the screen))
set file_list=%file_list% c.c d.d&rem

set /a cnt_total=0
set /a files_missing=0
for %%f in (%file_list%) do set /a cnt_total+=1

REM count number of files if you need this
echo cnt_total=%cnt_total%

for %%f in (%file_list%) do if not exist %%f echo File %%f is missing.&set files_missing=1

REM if files are missing just EXIT
if "%files_missing%"=="1" goto :EOF

REM delete temporary files
del a.a
del b.b
del c.c

Output:

Code: Select all

c:\>check_for_files.cmd
cnt_total=4
File d.d is missing.

c:\>

Saso