Letter used in a variable.

[PAB]

I am on virtually the last bit of finishing my file thanks to all the excellent help, responses, and direction from the experts here.

Besides the responses and advice given here in previous threads I have been doing my homework and my own investigations.

On https://ss64.com/nt/if.html it says . . .

String syntax . . . IF [/I] [NOT] item1==item2 command

So for example the %Drive% will be a letter . . .

Code: Select all

set "Drive="
set /p "Drive=>Please enter the drive letter and press <Enter>: "
if /i not exist %Drive%:\ goto :chkdsk_F_ERROR
if /i "%Drive%"=="C"      goto :chkdsk_F_C
if /i not "%Drive%"=="C"  goto :chkdsk_F_Other

I could also write them like this . . .

Code: Select all

if /i "%Drive%"=="C" (
  goto :chkdsk_F_C
) else (
  goto :chkdsk_F_Other
)

. . . or . . .

Code: Select all

if /i "%Drive%"=="C" (goto :chkdsk_F_C) else (goto :chkdsk_F_Other)

My question is, because they are letters that are used [ text string ], the speech marks are required as per the above! Is the code above correct please with regard to the speech marks?

Thanks in advance.

[Squashman]

The quotes are not required but are a best practice.

[penpen]

Sidenote: When using set/p (or when asking for input you don't conrol in general) you should always guard against invalid input.

For example your first codeblock here may enable others to execute any code he/she/... wants by typing input like the following:

Code: Select all

nul ( a ) else echo Selected Option D. & goto :eof & rem #

This could be done in the same way i recommended you guarding against non digit numbers:

Code: Select all

for /f "delims=ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" %%a in ("!Drive!") do ( echo(wrongInput & goto :eof)

penpen

[Aacini]

I think this is simpler:

Code: Select all

set "drives= ABCDEF"
choice /C %drives% /N /M ">Please enter the drive letter: "
set "drive=!drives:~%errorlevel%,1!"
rem Check the drive here...

This avoids all SET /P related problems and force the user to enter just one letter selected from a well defined letters set.

Antonio

[PAB]

Thank you BOTH for the replies, it is very much appreciated.

penpen

So using your code I would end up with . . .

Code: Select all

set "Drive="
set /p "Drive=>Please enter the drive letter and press <Enter>: "
for /f "delims=ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" %%a in ("!Drive!") do (echo wrongInput & goto :eof)
if /i %Drive%==C     goto :chkdsk_F_C
if /i not %Drive%==C goto :chkdsk_F_Other

In this bit . . .

Code: Select all

( echo(wrongInput

. . . was the space and extra bracket in your code intended, if so why please?

Aacini

So using your code I would end up with . . .

Code: Select all

set "Drives=ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
choice /C %Drives% /N /M ">Please enter the drive letter and press <Enter>: "
set "Drive=!Drives:~%errorlevel%,1!"
if /i %Drive%==C     goto :chkdsk_F_C
if /i not %Drive%==C goto :chkdsk_F_Other

. . . is the space before the A intended in your code, if so why please?

Thanks in advance.

[Aacini]

Mmmm... You entirely changed my code... Why you did that?

• In my answer I said that "the user enter just one letter selected from a well defined letters set" and put the letters "ABCDEF" as an example. Do you really want that the user can enter any letter from "A" to "Z"? Wouldn't be better to restrict the user to enter just the valid/allowable drive letters? I suppose that in :chkdsk_F_Other routine you check that the drive letter be valid. Wouldn't be simpler to do such a test directly in the input?
• Do you really want to process differently the upcase letters from the lowcase ones? This is the only reason to include both sets in the choice command... In this case you need to include the /CS switch in choice command...
• My code specify: ">Please enter the drive letter: ". You added "and press <Enter>". Why? You not need to press <Enter> when you use choice command.
• And yes, the space before A is intended in my code. And must be precisely one space. No zero spaces, no two or more spaces... You may eliminate the space in my code and test what happen...

I suggest you to use the SET /P based code instead...

Antonio

[pieh-ejdsch]

The errorlevel of chice is output exactly after the index of the characters.
The extension of variables is done according to the offset.
Why do you change the index to letter? Oh, you still need in the script. But if NOT
1 if - then it is also sufficient:

Code: Select all

set drives= abcde
choice /c %drives% /n /m ">please input drive letter"
if errorlevel 3 if NOT errorlevel 4 goto :chkdsk_f_c
call set "Drive=%%drives:~%errorlevel%,1%%"
:chkdsk_F_other
 rem some code
exit /b
:chkdsk_F_C
...

[edit] Oh Antonio was already faster

Phil

[aGerman]

Maybe something like that

Code: Select all

@echo off &setlocal EnableDelayedExpansion
set "valid="
for /f "tokens=1,2 delims=[]: " %%i in ('mountvol^|findstr "\<[A-Z]:\\\\"^|find /n /v ""') do set "drive_%%i=%%j" &set "valid=!valid!%%j"
choice /c !valid! /n /m "Enter a drive letter [!valid!]: "
set "drive=!drive_%errorlevel%!

if "!drive!"=="C" (
  echo Drive C chosen. (special case^)
) else (
  echo Drive !drive! chosen. (not C^)
)

pause

Steffen

[penpen]

So using your code I would end up with . . .

No, the for loop was meant as an example for what could go wrong.
So you also should guard against empty input and aborted input (pressing "CTRL+C" during set/p execution; which you correctly mapped to empty input by initializing the environment variable to be empty) as you did in your other code.
Another error could be that someone typed in more than just one letter.

Code: Select all

set "Drive="
set /p "Drive=>Please enter the drive letter and press <Enter>: "
for /f "delims=ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" %%a in ("!Drive!") do (echo wrongInput & goto :eof)
if "%Drive%" == ""    echo(emptyInput & goto :eof
if not "%Drive:~1%" == "" echo(twoOrMoreCharacters & goto :eof
if /i %Drive%==C     goto :chkdsk_F_C
if /i not %Drive%==C goto :chkdsk_F_Other

Also note that i don't know the flow of your program, so using "echo(errorMessage & goto :eof" might be not what you want to do.

. . . was the space and extra bracket in your code intended, if so why please?

Both were intended.
I added the extra space, so the code is easier to read.
I like "echo(" more, because on one hand it guards against some issues; see:
viewtopic.php?t=1900.
On the other hand its execution is faster because cmd.exe doesn't search for a file called echo.exe, echo.com, ... .


Sidenote:
When using choice as Antonio suggested (which should be the easier option as you need to guard against less issues), you also need to guard against aborted input (CTRL+C), by checking for:

Code: Select all

if %errorlevel% == 0 echo(abortedInput & goto :eof

penpen

[T3RRY]

A variant on the Choice for loop option I use when using single letter input to control script flow between labels that cleanly handles non existing labels:

Code: Select all

Echo/Select Drive:
For /F "Delims=" %%L in ('Choice /N /C:ABCDEFGHIJKLMNOPQRSTUVWXYZ') Do (Call :chkdsk_F_%%L || Call :chkdsk_F_other) 2> Nul
rem // script flow action if desired.
rem // return from chkdsk labels with errorlevel reset using: Exit /B 0

[PAB]

Thank you ALL for the replies.

I am a bit poorly at the moment so I am sorry that I have not got back to any of you.
When I feel a little better I will thoroughly go through this whole thread and take it all in, and answer each of your posts individually.
You are all brilliant at what you do.
I thought I would post all the ORIGINAL code before I started so you can see exactly what I had and what I was doing, which seems to work OK.
The drives could be any letter depending on how it was setup.
I think you are probably right about using CHOICE as it is easier to control wrong input!

Code: Select all

:chkdsk_F

cls
set "Drive="
echo. & echo  Which drive would you like to check for ERRORS? & echo.
set /p "Drive=>Enter the drive letter and press <Enter>: "
if /i not exist %Drive%:\ goto :chkdsk_F_ERROR
if /i "%Drive%"=="C"      goto :chkdsk_F_C
if /i not "%Drive%"=="C"  goto :chkdsk_F_Other

:chkdsk_F_ERROR

echo. & echo  ERROR: The drive does NOT exist^^!
echo. & echo ^>Press ANY key to return to the Menu . . . & pause >nul
cls & goto :Menu

:chkdsk_F_C

echo. & echo  Running chkdsk %Drive%: /F . . .
        echo  ----------------------------------------------------------------------------------------
echo. &       chkdsk %Drive%: /F
echo. & echo  ----------------------------------------------------------------------------------------
echo. & echo  If you DON'T want to REBOOT NOW, press ^<Enter^> to return to the Menu.
        echo  If you DO    want to REBOOT NOW, press ^<Y^> and ^<Enter^>.
        echo  ----------------------------------------------------------------------------------------
echo. & goto :REBOOT_Choice

:chkdsk_F_Other

echo. & echo  Running chkdsk %Drive%: /F . . .
        echo  ----------------------------------------------------------------------------------------
echo. &       chkdsk %Drive%: /F
echo. & echo  ----------------------------------------------------------------------------------------
        echo  IMPORTANT:
echo. & echo     Please thoroughly check the results.
        echo  ----------------------------------------------------------------------------------------
echo. & echo ^>Press ANY key to return to the Menu . . . & pause >nul
cls & goto :Menu

As I said, once I am feeling better in a couple of days hopefully, I want to really get this sorted.

Thanks again everyone, it is appreciated.

[T3RRY]

fyi, choice does accept numbers 0-9 in addition to alphabetical characters. Note using a for /f loop to iterate over the choice command will return letters in upper case, even if the choice options are declared using lower case.

You can also obtain the available drives using wmic to define (and display) the valid drive options:

Code: Select all

@Echo off & Setlocal DisableDelayedExpansion
(Set \n=^^^
%=Newline DNR=%
)

Set CHECKDISK=For %%n in (1 2) Do If %%n==2 (%\n%
  For /F "Tokens=1 Delims={}" %%1 in ("!Key!") Do (%\n%
   echo/^& echo  Run chkdsk %%~1: /F Y/N?%\n%
   For /F "Delims=" %%. in ('Choice /N /C:yn') Do If "%%."=="N" Endlocal^&Goto :Menu%\n%
   echo/^& echo  Running chkdsk %%~1: /F . . .%\n%
   echo/----------------------------------------------------------------------------------------%\n%
   echo/^&chkdsk %%~1: /F%\n%
   echo/ ^& echo/----------------------------------------------------------------------------------------%\n%
   echo/IMPORTANT:%\n%
   echo/^& echo/Please thoroughly check the results.%\n%
   echo/----------------------------------------------------------------------------------------%\n%
   echo/^& echo/^^>Press ANY key to return to the Menu . . . ^& pause ^>nul%\n%
  )%\n%
) Else Set Key=

:Menu
Goto :CheckDisk
:CheckDisk
Setlocal EnableDelayedExpansion
    Set AvDrives=
For /f "tokens=2 delims=:=" %%f in ('wmic logicaldisk get name /value') do (
rem Build a reference string containing the drives returned to use as the Array index.
    Set "AvDrives=!AvDrives!%%f"
)

Echo/Menu 0 Drives:!AvDrives!
For /F "Delims=" %%O in ('Choice /N /C:!AvDrives!0') Do If Not "%%O" == "0" (%CHECKDISK%{%%O}) Else (Endlocal&Goto :Menu)
Endlocal&Goto :Menu

[aGerman]

wmic

Good idea but you should filter the drives types. E.g. network drives are probably not sensible here.

Steffen

[PAB]

fyi, choice does accept numbers 0-9 in addition to alphabetical characters. Note using a for /f loop to iterate over the choice command will return letters in upper case, even if the choice options are declared using lower case.

You can also obtain the available drives using wmic to define (and display) the valid drive options:

Code: Select all

@Echo off & Setlocal DisableDelayedExpansion
(Set \n=^^^
%=Newline DNR=%
)

Set CHECKDISK=For %%n in (1 2) Do If %%n==2 (%\n%
  For /F "Tokens=1 Delims={}" %%1 in ("!Key!") Do (%\n%
   echo/^& echo  Run chkdsk %%~1: /F Y/N?%\n%
   For /F "Delims=" %%. in ('Choice /N /C:yn') Do If "%%."=="N" Endlocal^&Goto :Menu%\n%
   echo/^& echo  Running chkdsk %%~1: /F . . .%\n%
   echo/----------------------------------------------------------------------------------------%\n%
   echo/^&chkdsk %%~1: /F%\n%
   echo/ ^& echo/----------------------------------------------------------------------------------------%\n%
   echo/IMPORTANT:%\n%
   echo/^& echo/Please thoroughly check the results.%\n%
   echo/----------------------------------------------------------------------------------------%\n%
   echo/^& echo/^^>Press ANY key to return to the Menu . . . ^& pause ^>nul%\n%
  )%\n%
) Else Set Key=

:Menu
Goto :CheckDisk
:CheckDisk
Setlocal EnableDelayedExpansion
    Set AvDrives=
For /f "tokens=2 delims=:=" %%f in ('wmic logicaldisk get name /value') do (
rem Build a reference string containing the drives returned to use as the Array index.
    Set "AvDrives=!AvDrives!%%f"
)

Echo/Menu 0 Drives:!AvDrives!
For /F "Delims=" %%O in ('Choice /N /C:!AvDrives!0') Do If Not "%%O" == "0" (%CHECKDISK%{%%O}) Else (Endlocal&Goto :Menu)
Endlocal&Goto :Menu

Thank you so much. I will test that out over the next couple of days and report back.

[PAB]

Code: Select all

@Echo off & Setlocal DisableDelayedExpansion
(Set \n=^^^
%=Newline DNR=%
)

Set CHECKDISK=For %%n in (1 2) Do If %%n==2 (%\n%
  For /F "Tokens=1 Delims={}" %%1 in ("!Key!") Do (%\n%
   echo/^& echo  Run chkdsk %%~1: /F Y/N?%\n%
   For /F "Delims=" %%. in ('Choice /N /C:yn') Do If "%%."=="N" Endlocal^&Goto :Menu%\n%
   echo/^& echo  Running chkdsk %%~1: /F . . .%\n%
   echo/----------------------------------------------------------------------------------------%\n%
   echo/^&chkdsk %%~1: /F%\n%
   echo/ ^& echo/----------------------------------------------------------------------------------------%\n%
   echo/IMPORTANT:%\n%
   echo/^& echo/Please thoroughly check the results.%\n%
   echo/----------------------------------------------------------------------------------------%\n%
   echo/^& echo/^^>Press ANY key to return to the Menu . . . ^& pause ^>nul%\n%
  )%\n%
) Else Set Key=

:Menu
Goto :CheckDisk
:CheckDisk
Setlocal EnableDelayedExpansion
    Set AvDrives=
For /f "tokens=2 delims=:=" %%f in ('wmic logicaldisk get name /value') do (
rem Build a reference string containing the drives returned to use as the Array index.
    Set "AvDrives=!AvDrives!%%f"
)

Echo/Menu 0 Drives:!AvDrives!
For /F "Delims=" %%O in ('Choice /N /C:!AvDrives!0') Do If Not "%%O" == "0" (%CHECKDISK%{%%O}) Else (Endlocal&Goto :Menu)
Endlocal&Goto :Menu

That works perfectly thank you.

What does . . .
[1] The %\n% [ Set \n=^^^ ] at then end of each line do and . . .
[2] The echo/ do please?

Is [2] the same as echo.?

Thanks in advance.