Please review my code

Message

timothymycompc · #1 Post by **timothymycompc** » 20 Apr 2021 20:57

Hello im new to here and I am just learning how to script in batch files.
I work at a r2 company and we have very strict procedures for gathering data and wipeing data
I created some scripts to help me along but it feel like i just got my feet wet.
I want to get involved in a community that can help me learn or show me better ways to do things
below im going to attach my files ive worte so far let me know what yall think and ways to improve
or brainstorm ideas or how i could have don something better

First script

Code: Select all

@echo off
WMIC CSPRODUCT GET NAME > MODEL.TXT


for /f "DELIMS=" %%i in ('MORE MODEL.txt') do set model="%%i"

DEL MODEL.TXT 

md %Model%
cd %model%

wmic diskdrive get size > HARDRIVESIZE.TXT
systeminfo | findstr /C:"Total Physical Memory" > MEMSIZE.TXT
WMIC CSPRODUCT GET NAME > MODEL.TXT
wmic cpu get NAME > CPU.TXT
WMIC BIOS GET SERIALNUMBER > SERIAL.TXT



for /f "delims=" %%i in ('more HARDRIVESIZE.txt') do set hdsize=%%i
for /f "DELIMS=" %%i in ('MORE MEMSIZE.txt') do set MEM=%%i
for /f "DELIMS=" %%i in ('MORE MODEL.txt') do set MODeL=%%i
for /F "DELIMS=" %%i in ('MORE CPU.txt') do set CPU=%%i
for /F "DELIMS=" %%i in ('MORE SERIAL.txt') do set SERIAL=%%i


setlocal enabledelayedexpansion 
set obj[0].desc=model
set obj[0].size=%model%
set obj[1].desc=serial
set obj[1].size=%SERIAL%
set obj[2].desc=cpu
set obj[2].size=%CPU%
set obj[3].desc=memory
set obj[3].size=%MEM%
set obj[4].desc=hardrive
set obj[4].size=%HDSIZE%

for /l %%i in (0 1 4) do (
call echo %%obj[%%i].desc%%
call echo %%obj[%%i].size%%
) >> info.txt
 

ECHO " <!DOCTYPE html>" >> info.HTML
ECHO " <html>" >> info.HTML
ECHO " <body>" >> info.HTML

ECHO " <h2>%model% info</h2>" >> info.HTML
ECHO " <table style="width:100%">" >> info.HTML

ECHO " <td></td>" >> info.HTML
ECHO " <tr>" >> info.HTML
ECHO " </tr>" >> info.HTML
ECHO " <td></td>" >> info.HTML

ECHO "   <tr>" >> info.HTML
ECHO "     <th></th>" >> info.HTML
ECHO "     <th></th>" >> info.HTML
ECHO "   </tr>" >> info.HTML

ECHO " <td></td>" >> info.HTML
ECHO " <tr>" >> info.HTML
ECHO " </tr>" >> info.HTML
ECHO " <td></td>" >> info.HTML

ECHO "<tr>" >> info.HTML
ECHO "<td>Serial:</td>" >> info.HTML
ECHO "<td>%serial%</td>" >> info.HTML
ECHO "<td></td>" >> info.HTML
ECHO "</tr>" >> info.HTML

ECHO " <td></td>" >> info.HTML
ECHO " <tr>" >> info.HTML
ECHO " </tr>" >> info.HTML
ECHO " <td></td>" >> info.HTML

ECHO "<tr>" >> info.HTML
ECHO "<td>HardDriveSize:</td>" >> info.HTML
ECHO "<td>%HDSIZE%</td>" >> info.HTML
ECHO "<td></td>" >> info.HTML
ECHO "</tr>" >> info.HTML

ECHO " <td></td>" >> info.HTML
ECHO " <tr>" >> info.HTML
ECHO " </tr>" >> info.HTML
ECHO " <td></td>" >> info.HTML

ECHO "<tr>" >> info.HTML
ECHO "<td>Cpu:</td>" >> info.HTML
ECHO "<td>%cpu%</td>" >> info.HTML
ECHO "<td></td>" >> info.HTML
ECHO "</tr>" >> info.HTML

ECHO " <td></td>" >> info.HTML
ECHO " <tr>" >> info.HTML
ECHO " </tr>" >> info.HTML
ECHO " <td></td>" >> info.HTML

ECHO "<tr>" >> info.HTML
ECHO "<td>Model:</td>" >> info.HTML
ECHO "<td>%model%</td>" >> info.HTML
ECHO "<td></td>" >> info.HTML
ECHO "</tr>" >> info.HTML

ECHO " <td></td>" >> info.HTML
ECHO " <tr>" >> info.HTML
ECHO " </tr>" >> info.HTML
ECHO " <td></td>" >> info.HTML

ECHO "<tr>" >> info.HTML
ECHO "<td>Memory:</td>" >> info.HTML
ECHO "<td>%mem%</td>" >> info.HTML
ECHO "<td></td>" >> info.HTML
ECHO "</tr>" >> info.HTML

ECHO " <td></td>" >> info.HTML
ECHO " <tr>" >> info.HTML
ECHO " </tr>" >> info.HTML
ECHO " <td></td>" >> info.HTML

ECHO " <td></td>" >> info.HTML
ECHO " <tr>" >> info.HTML
ECHO " </tr>" >> info.HTML
ECHO " <td></td>" >> info.HTML

ECHO " </table>" >> info.HTML
ECHO " </body>" >> info.HTML
ECHO " </html>" >> info.HTML


DEL MEMSIZE*
DEL MODEL*
DEL CPU*
DEL SERIAL*
del HARDR*

cd ..

Second script

Code: Select all

@echo off
cd phonesinfo 
adb shell getprop ro.serialno > serial.txt
adb shell getprop ro.boot.serialno  >> serial.txt
adb shell getprop ro.product.model > model.txt
adb shell getprop ro.product.vendor.manufacturer > manuf.txt
adb shell getprop ro.carrier > carrier.txt
adb shell getprop ro.logd.size.stats > memsize.txt 
adb shell getprop ril.serialnumber > othermodel.txt
adb shell df /data > hdsize.txt
for /f "DELIMS=" %%i in ('MORE othermodel.txt') do set otmodel="%%i"
for /f "tokens=2" %%i in ('MORE hdsize.txt') do set hdsize="%%i"
for /f "DELIMS=" %%i in ('MORE manuf.txt') do set manuf="%%i"
for /f "DELIMS=" %%i in ('MORE serial.txt') do set seri="%%i"
for /f "DELIMS=" %%i in ('MORE MODEL.txt') do set model="%%i"
for /f "DELIMS=" %%i in ('MORE carrier.txt') do set carr="%%i"
for /f "DELIMS=" %%i in ('MORE memsize.txt ') do set mem="%%i"

del *.txt
md %model%.%seri%
cd %model%.%seri%
echo "" > %model%.%seri%.txt
echo hdsize= %hdsize% >> %model%.%seri%.txt
echo manufacturer= %manuf% >> %model%.%seri%.txt
echo model= %model% >> %model%.%seri%.txt
echo memory= %mem% >> %model%.%seri%.txt
echo carrier= %carr% >> %model%.%seri%.txt
echo serial= %seri% >> %model%.%seri%.txt
echo othermodel= %otmodel% >> %model%.%seri%.txt
adb shell getprop > %model%.%seri%.getprop.txt
echo "this program is ment to wipe a phone"
echo "if at any time anything dose not look right just hit ctrl + c it will ask if you wanna terminate select yes"
set /p as="in order to reboot into bootloader and wipe phone press enter to continue (you may have to still wipe manually"
adb reboot recovery

Maylow · #2 Post by **Maylow** » 21 Apr 2021 10:55

At first sight, I'd suggest an improvement would be to verify/validate data before assigning to variables.
For example, the output of your first wmic statement contains multiple lines, first line contains name of field, second line contains data, third line contains empty space.
Is uppose you only want the second line to be assigned to a variable.

Also, if you want to use the output of executables/command, instead of writing the output to a file and then read the output file, this can easily be achieved with the FOR statements themselves.
This saves a lot of HD I/O and thus execution time.
This is done with backticks `.
Look at the FOR /? help for more information.

For example, the following code:
WMIC CSPRODUCT GET NAME > MODEL.TXT
for /f "DELIMS=" %%i in ('MORE MODEL.txt') do set model="%%i"

Could be changed into:

Code: Select all

for /f "usebackq tokens=*" %%i in (
    `WMIC CSPRODUCT GET NAME`) do (
    set "model=%%~i"
)

However, as in this case the wmic command gives output with multiple lines where the second line contains the data, you might want to do something like:

Code: Select all

setlocal enableDelayedExpansion
set wmic.I=0
for /f "usebackq tokens=*" %%i in (
    `WMIC CSPRODUCT GET NAME`) do (
    set /a wmic.I+=1
    if "!wmic.I!" equ "2" (
        endlocal
        set "model=%%~i"
    )
)
if "!!" equ "" endlocal

Same goes for the other wmic statements and for systeminfo:
systeminfo | findstr /C:"Total Physical Memory" > MEMSIZE.TXT
for /f "DELIMS=" %%i in ('MORE MEMSIZE.txt') do set MEM=%%i

Could be changed into:

Code: Select all

setlocal enableDelayedExpansion
for /f "usebackq tokens=*" %%i in (
    `systeminfo ^| findstr /C:"Total Physical Memory"`) do (
    for /f "tokens=1,* delims=:" %%u in ("%%~i") do (
        set "MEM=%%~v"
        set "MEM=!MEM: =!"
    )
)
endlocal & set "MEM=%MEM%"

I'd strongly suggest to examine the output of each executable/command you have used, assign only the requested data and filter out any unnecessary data.

As for the DEL statements, I would perform more intensive checks to ensure no unintended files are deleted.
Your current statements like 'DEL MEMSIZE*' now deletes everything in the current directory starting with MEMSIZE*, also other filenames and other filetypes.
When executing the script with full path from another directory, other files might unintendedly be deleted.
It is a good practice in common to be certain of the files you are about to change/delete.
For example, you can use the PUSHD and POPD commands to change the current active directory to the script's directory:

Code: Select all

@echo off
pushd "%~dp0"

*code*

popd
exit /b

This can be a way to be sure you only delete files in that directory.
This does not give enough guarantees imho.
Edit: another way to ensure only the files you used are deleted is to use lists with filenames:

Code: Select all

REM Define used files:
set "output_files=filename1.txt filename2.txt"

REM *code* - which writes to "%~sdp0filename1.txt", "%~sdp0filename1.txt", et cetera.

REM Delete used files:
for %%b in (%output_files%) do (
    if exist "%~sdp0%%~b" del "%~sdp0%%~b"
)

Personally, I always end scripts/routines with the EXIT /B or GOTO:EOF statement, unless there is a specific reason not to do it.

I can't say anything about the 'adb shell getprop' statement as I don't have adb to test it.
But my guess is that it can also better be captured with the FOR /F "usebackq" statement.

I hope this helps!

DosTips.com

Please review my code

Please review my code

Re: Please review my code