Need comments on my first ever DOS script program

[jonathanjohnson100]

Took me about 2 hours to write some DOS code

I want to rename all jpg images to the same name as the parent directory (with a number added at the end)

I wanted comments on my code and whether it is optimal without any bugs?

This is what I have:

Code: Select all

setlocal EnableDelayedExpansion

REM Get just the name of parent directory - and not the full path
FOR /D %%I IN ("%CD%") DO SET _LAST_SEGMENT_=%%~nxI

set /a i=1

for /f "delims=" %%a in (' dir *.jpg /b /a-d ')  do (

ren "%%a" "%_LAST_SEGMENT_% !i!.jpg"
set /a i=i+1
)

Pause

echo off has been intentionally left off - I'll add when finished.

Pause has been added for now, finished version won't have.

Thanks!

JJ

[penpen]

You could use "::" instead of "REM", avoide using environment variables (use inner and outer loop), "for" instead "for /f" (then you also need to use %%~a instead of %%a; see "for /?"), and shorter "set /a" commands.

Code: Select all

setlocal EnableDelayedExpansion

:: Get just the name of parent directory - and not the full path
FOR /D %%I IN ("%CD%") DO SET _LAST_SEGMENT_=%%~nxI

set i=1
for %%I in ("%_LAST_SEGMENT_%") do for %%a in (*.jpg) do (
   echo ren "%%~a" "%%~I !i!.jpg"
   set /a i+=1
)

Pause

(The comment may be rewritten; i've echoed the ren command instead renaming the files for debugging.)
I'm not sure if you see this as a bug, but you won't get a parent directory name on root directories.

penpen

[Compo]

Whilst there was nothing 'wrong' in your code, here's a re-write with comments to help you.

Code: Select all

@Echo off & SetLocal EnableExtensions EnableDelayedExpansion

Rem Takes an input directory to process otherwise uses script directory
If "%~1" Equ "" (Set _input=%~dp0) Else (Set _input=%~1)

Rem Ensures process directory is valid and current
PushD %_input%||GoTo :EndIt

Rem Sets a variable for the parent directory name 
For %%A In ("%CD%") Do (Set _parent=%%~nxA)

Rem ReSets the variable if the parent directory is a drive
If "%_parent%" Equ "" (Set _parent=%CD:~,1%)

Rem Asks end user before processing
Set/p _answer=Are you sure you wish to rename all jpg's in %_parent% [Y\N]

Rem continues according to response
If /i %_answer% NEq Y GoTo :EndIt

Rem Sets a variable for the start of the number count
(Set _n=1)

Rem loops through all jpg's in input directory and renames to specification
For %%A In (*.jpg) Do Ren "%%~A" "%_parent% !_n!%%~xA" & (Set/a _n+=1)

:EndIt
PopD & EndLocal & Pause

[ShadowThief]

You could use "::" instead of "REM"

Note that you can not use :: to comment inside of for loops because :: is technically a label.

[foxidrive]

Note that you can not use :: to comment inside of for loops because :: is technically a label.

They still work though - with the exception of directly above a closing bracket in (some?) loops.

[penpen]

:: is technically a label

Are you sure? I'm not sure (maybe "::" is handled in a different way, when using in for loops):

Code: Select all

@echo off
for %%a in (1 2 3) do (
   :: no label
   :: no label
   echo %%a
)

Result:

Code: Select all

Z:\>test
Das System kann das angegebene Laufwerk nicht finden.
1
Das System kann das angegebene Laufwerk nicht finden.
2
Das System kann das angegebene Laufwerk nicht finden.
3

I am using a german version of win xp home 32 bit, so the error message should be something like:
"The system cannot find the drive specified."

But it is possible to use a single "::" style comment (prior a following instruction line); if you want to use more lines you have to use the caret to make it one line:

Code: Select all

@echo off
for %%a in (1 2 3) do (
   :: no label^
   :: no label
   echo %%a
)

In addition (with a trick) a label in the last line is possible, while a "::" style comment is impossible (without an error):

Code: Select all

@echo off
echo(last line in for: label
for %%a in (1 2 3) do (
   echo(%%a
   ^:: no label
   :label
)

echo(
echo(last line in for: no label
for %%a in (1 2 3) do (
   echo(%%a
   ^:label
   :: no label
)

At last please note that you cannot use some alternative echo formats, such as "echo(%%a" normally working in for loops, when using a "::" line prior:

Code: Select all

@echo off
echo(without comment
for %%a in (1 2 3) do (
   echo %%a.1
   echo(%%a.2
   echo.%%a.3
)

echo(
echo(with comment
for %%a in (1 2 3) do (
   :: 1
   echo %%a.1
   :: 2
   echo(%%a.2
   :: 3
   echo.%%a.3
)

penpen

[ShadowThief]

Note that you can not use :: to comment inside of for loops because :: is technically a label.

They still work though - with the exception of directly above a closing bracket in (some?) loops.

I've never been able to get :: to work inside of loops regardless of placement.

The real point here is that :: inside of loops can cause unexpected behavior and should be avoided.