Skip to content

CODE_REVIEW.en.md

Latest commit

 

History

History
246 lines (187 loc) · 9.66 KB

CODE_REVIEW.en.md

File metadata and controls

246 lines (187 loc) · 9.66 KB
Raw

Code review (2026-07-31)

한국어 | English

A full review of the ten original 2019 scripts, and a record of what this revision changed and why. The point is to preserve the reasoning, so every defect quotes the original code.

The originals are reachable with git log --follow <file>.

1. Defects that stopped the code from working

1-1. DateFormateCustom.vbs — the function always returned an empty value

Function DateFormateCustom(vTempInputDate, vTempStandard)
  ...
  DateCustom = vTempYear & vTempStandard & vTempMonth & vTempStandard & vTempDay
End Function

In VBScript a function returns a value only when you assign to the function's own name. DateCustom is not DateFormateCustom, so this created a throwaway local and every caller received an empty value. With no Option Explicit, the typo passed silently.

The same file held two more defects.

If InStr(vTempDateString, "/") <> 0 Then      ' the parameter is vTempInputDate
  vTempSplit = "/"
ElseIf InStr(vTempDateString, "-") <> 0 Then
  vTempSplit = "_"                            ' finds "-" but splits on "_"
  • vTempDateString exists nowhere, so it is always Empty, all three conditions are false, and vTempSplit stays an empty string. Split(value, "") returns the whole input as a single element, so the month/day/year decomposition never happened at all.
  • When the separator was -, the code split on _ (typo).

Fix: renamed to FormatCustomDate with the return assignment matching the function name, separator detection extracted into DetectDelimiter, the h:m:s tail dropped, month and day zero padded, two digit years expanded, and an empty string returned on unparseable input instead of raising.

1-2. SapConnection.vbs — logged on with an empty password

Dim vIP, vLanguage, vUserName, vPassWord, ...
vPw = WScript.Arguments.Item(3)                       ' read into vPw
...
vSession.findById(".../pwdRSYST-BCODE").text = vPassWord   ' writes vPassWord

The argument landed in vPw while the screen received vPassWord, which was never assigned. An empty password was sent every time. Missing Option Explicit again.

vSession.findById("wnd[0]").maximizea    ' typo for maximize

Fix: unified the variable, corrected maximize, added Option Explicit.

1-3. ReservationExcelEdit.vbs — printed "Fail" even on success

WScript.StdOut.WriteLine(vMaxRow)

Err.Source 6                          ' Err.Source is a property; this call form does not exist
WScript.StdOut.WriteLine("Fail")      ' runs unconditionally
Err.Clear

If Err.Number <> 0 Then was presumably the intent. What actually happens is that a second line reading Fail is printed straight after the row count. The blanket On Error Resume Next at the top swallowed the error raised by Err.Source 6 itself, so nobody noticed.

Fix: removed the blanket On Error Resume Next, put only the row count on stdout, and moved failures to stderr with exit codes 1–4.

1-4. ReservationExcelEdit.vbs — infinite loop on an empty sheet

Function setValue(vTempSheet, vTempColun, vTempMaxRow, vTempSupplier)
  vTempRow = 2
  Do
    vTempSheet.Cells(vTempRow, vTempCol).Value = vTempSupplier
    vTempRow = vTempRow + 1
    vTempMaxRow = vTempMaxRow - 1
  Loop While vTempMaxRow <> 0
End Function

Do ... Loop While is post-tested, so the body always runs at least once. When vTempMaxRow arrives as 0, one cell is written, the counter drops to −1, and <> 0 is true forever. On a sheet with no data this writes cells until Excel runs out of rows.

Fix: switched to a pre-tested Do While and an early return on RowCount < 1. While there, the per-cell COM loop was replaced with a single Range(...).Value assignment — a large difference at a few thousand rows.

1-5. ReservationExcelEdit.vbs — mojibake broke the column lookup

The file was stored as UTF-8 without a BOM. cscript.exe reads .vbs using the system code page (CP949 on Korean Windows), so the literals "플랜트", "납품처" and "생산버전" all arrived corrupted. Range.Find matched nothing, FindColume returned 0, and the subsequent Cells(row, 0) access failed.

Fix: pinned the working tree encoding to UTF-16LE + BOM via working-tree-encoding in .gitattributes. The repository blob stays UTF-8, so GitHub renders it and diffs still work.

1-6. ReservationExcelEdit.vbs — indexed the workbook by path

vExcelPath = WScript.Arguments.Item(0)         ' a full path
Set objWorkbook = objexcel.Workbooks(vExcelPath)

The Workbooks collection is indexed by file name. Passing a full path always fails.

Fix: accept a path but look up by its leaf, and fail explicitly with exit code 3 when it is not found.

1-7. ExcelOpen — the path to open was commented out

'vExcelPath = WScript.Arguments.Item(0)
...
Set objWorkbook = objExcel.Workbooks.Open(vExcelPath)   ' opens an empty path

Fix: restored argument parsing, added an existence check, usage text and exit codes. Gave the file a .vbs extension.

1-8. log write — smart quotes made it unrunnable

Set objFileToWrite = CreateObject(“Scripting.FileSystemObject”)...

A note that had been through a word processor was committed as-is, leaving the quotes as U+201C/U+201D and the comment marker as U+2018. Pasting it produces a syntax error immediately.

Fix: rewritten as working code in FileLog.vbs. Declares the constants (FOR_APPENDING and friends are not built in to VBScript, unlike VBA) and guards against ReadAll raising on an empty file.

1-9. Deduplication.vbs — substring matching

For Each x In Split(vTempString, vTempStandard)
  If InStr(vCopyString, x) <> 0 Then
    vTempStringOne = vTempStringOne & "," & x
    vCopyString = Replace(vCopyString, x, "")

This is correct only for single character tokens, like the example in the header comment ("a,a,c,c,..."). InStr and Replace work on substrings, so with codes such as "1000,1010" removing 1000 also damages part of 1010. The result was also joined with a hard coded , rather than the vTempStandard parameter.

Fix: exact token matching through Scripting.Dictionary (vbTextCompare), and the delimiter parameter is actually used.

1-10. copy_folder_aaefolder.vbs — a MsgBox in an unattended script

If vFSO.FolderExists(vFolderPath) Then
Else
	Msgbox(vFolderPath)          ' waits forever on an unattended PC
	vFSO.CreateFolder vFolderPath
End If

The script is launched from a .bat on a schedule, yet it opens a modal dialog. With nobody there to answer, the backup simply stops. Also:

  • vFolderPath = "C:\" followed by & "\" & produced C:\\2019_7_5
  • No zero padding, so 2019_10_1 sorts before 2019_2_1
  • C:\Users\powergen\... hard coded, and writing to the C:\ root needs admin
  • If ... Then Else with an empty Then branch

Fix: renamed to BackupFolder.vbs, dropped the MsgBox, moved paths to arguments, used BuildPath, zero padded the stamp, and added a trailing \ to the Folder.Copy target so "copy into" is unambiguous.

2. Repository-wide quality problems

Item Original Fix
Option Explicit absent everywhere → the direct cause of 1-1 and 1-2 declared in every file
Undeclared variables vCount, vFolderPath, FoundCell, table, row, x … all Dimed
Function with no return MultiInput, setValue converted to Sub
Global coupling SapTableInput assumed a global session vSession parameter
Error handling blanket On Error Resume Next, or none per-failure exit codes + stderr
Unbounded waits the three SAP connection loops had no timeout MAX_ATTEMPTS 60 (30 s)
File names DateFormateCustom (typo), two with no extension, log write with a space corrected, .vbs added
Password command line argument environment variable / PSCredential

The index in SapTableInput.vbs

Call VerticalScrolling(tempTwo, vCount)
Set table = session.findById(tempTwo)
Set row = table.getcell(1, 1)          ' zero based, but 1,1

GuiTableControl.GetCell(row, column) is zero based and addresses the visible region after a scroll. Scrolling one row at a time and then writing to (1, 1) skips the first visible row and writes to the second. Corrected to (0, columnIndex), with the column as a parameter.

The original behaviour of re-resolving the control with findById after each scroll is correct — SAP GUI rebuilds its child elements when the viewport moves, so the earlier reference goes stale. The reason is now stated in a comment.

3. Remaining limitations (worth knowing)

  • Not verified at runtime. This was a static review. SAP GUI and Excel COM need a real Windows environment, so nothing here was executed. §1-5 (encoding) and the zero based index in SapTableInput in particular are worth confirming with a single run against the real system.
  • FormatCustomDate is fixed to m/d/y, following the input format stated in the original header comment. Feeding it d/m/y locale data silently produces wrong values. The PowerShell counterpart (ConvertTo-SapDate.ps1) reports a parse failure as a non-terminating error instead.
  • Deleting column C in ReservationExcelEdit.vbs is positional, not header based, so a change in the export's column order wipes the wrong column. The original behaviour was kept, but a header based lookup would be safer.
  • git ≥ 2.21 required. Cloning with an older git that does not support working-tree-encoding checks ReservationExcelEdit.vbs out as UTF-8. In that case, re-save it as "UTF-16 LE" in an editor.