Skip to content

Commit 5cb6b06

Browse files
randleerandlee
committed
refactor: Replace List with IReadOnlyCollection for error handling and improve null checks
Standardized error handling interfaces to use `IReadOnlyCollection`. Revised test assertions to use `First()` instead of index access, and added additional null checks for robustness.
1 parent 0067e43 commit 5cb6b06

File tree

7 files changed

+49
-44
lines changed

7 files changed

+49
-44
lines changed

‎src/MDTool/Core/MarkdownParser.cs‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,13 +33,11 @@ public MarkdownParser()
3333
/// </summary>
3434
/// <param name="content">The markdown content string to parse</param>
3535
/// <returns>ProcessingResult containing MarkdownDocument or errors</returns>
36-
public ProcessingResult<MarkdownDocument> ParseContent(string content)
36+
public ProcessingResult<MarkdownDocument> ParseContent(string? content)
3737
{
3838
if (content == null)
3939
{
40-
return ProcessingResult<MarkdownDocument>.Fail(
41-
ValidationError.ProcessingError("Content cannot be null")
42-
);
40+
return ProcessingResult<MarkdownDocument>.Fail( ValidationError.ProcessingError("Content cannot be null") );
4341
}
4442

4543
var errors = new List<ValidationError>();

‎src/MDTool/Models/ProcessingResult.cs‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,12 @@ public class ProcessingResult<T>
2020
/// <summary>
2121
/// List of errors (only populated if Success is false).
2222
/// </summary>
23-
public List<ValidationError> Errors { get; init; }
23+
public IReadOnlyCollection<ValidationError> Errors { get; init; }
2424

2525
/// <summary>
2626
/// Private constructor - use factory methods.
2727
/// </summary>
28-
private ProcessingResult(bool success, T value, List<ValidationError> errors)
28+
private ProcessingResult(bool success, T value, IReadOnlyCollection<ValidationError> errors)
2929
{
3030
Success = success;
3131
Value = value;
@@ -50,17 +50,19 @@ public static ProcessingResult<T> Ok(T value)
5050
/// <summary>
5151
/// Creates a failed result with errors.
5252
/// </summary>
53-
public static ProcessingResult<T> Fail(List<ValidationError> errors)
53+
public static ProcessingResult<T> Fail(IEnumerable<ValidationError> errors)
5454
{
55-
if (errors == null || errors.Count == 0)
55+
// ReSharper disable PossibleMultipleEnumeration
56+
if (errors == null || !errors.Any())
5657
throw new ArgumentException("Failed result must have errors", nameof(errors));
5758

5859
// ReSharper disable once NullableWarningSuppressionIsUsed
5960
return new ProcessingResult<T>(
6061
success: false,
6162
value: default!,
62-
errors: errors
63+
errors: errors.ToArray()
6364
);
65+
// ReSharper restore PossibleMultipleEnumeration
6466
}
6567

6668
/// <summary>

‎src/MDTool/Utilities/JsonOutput.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ public static string Success<T>(T result)
6767
/// </summary>
6868
/// <param name="errors">List of validation or processing errors</param>
6969
/// <returns>JSON string with success=false and error details</returns>
70-
public static string Failure(List<ValidationError> errors)
70+
public static string Failure(IReadOnlyCollection<ValidationError> errors)
7171
{
7272
try
7373
{

‎tests/MDTool.Tests/Core/VariableSubstitutorTests.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ public void Substitute_PartiallyProvidedVariables_ReportsOnlyMissing()
224224

225225
Assert.False(result.Success);
226226
Assert.Single(result.Errors);
227-
Assert.Contains("EMAIL", result.Errors[0].Variable);
227+
Assert.Contains("EMAIL", result.Errors.First().Variable);
228228
}
229229

230230
[Fact]

‎tests/MDTool.Tests/Integration/Wave2IntegrationTests.cs‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ public async Task Integration_FileHelper_MarkdownParser_SuccessCase()
5656
// Assert: Verify the pipeline worked
5757
Assert.True(parseResult.Success, "Parse should succeed");
5858
var doc = parseResult.Value;
59-
Assert.NotNull(doc.Variables);
59+
Assert.NotNull(doc?.Variables);
6060
Assert.Equal(2, doc.Variables.Count);
6161
Assert.True(doc.Variables.ContainsKey("USER_NAME"));
6262
Assert.Contains("{{USER_NAME}}", doc.Content);
@@ -75,8 +75,8 @@ public async Task Integration_FileHelper_MarkdownParser_FileNotFoundError()
7575
// Assert: FileHelper error should propagate correctly
7676
Assert.False(readResult.Success, "File read should fail");
7777
Assert.Single(readResult.Errors);
78-
Assert.Equal(ErrorType.FileNotFound, readResult.Errors[0].Type);
79-
Assert.Contains("not found", readResult.Errors[0].Description, StringComparison.OrdinalIgnoreCase);
78+
Assert.Equal(ErrorType.FileNotFound, readResult.Errors.First().Type);
79+
Assert.Contains("not found", readResult.Errors.First().Description, StringComparison.OrdinalIgnoreCase);
8080
}
8181

8282
[Fact]
@@ -101,7 +101,7 @@ public async Task Integration_FileHelper_MarkdownParser_InvalidMarkdownError()
101101
// Assert: Parser error should be returned
102102
Assert.False(parseResult.Success, "Parse should fail");
103103
Assert.Single(parseResult.Errors);
104-
Assert.Equal(ErrorType.InvalidYamlHeader, parseResult.Errors[0].Type);
104+
Assert.Equal(ErrorType.InvalidYamlHeader, parseResult.Errors.First().Type);
105105
}
106106

107107
#endregion
@@ -229,10 +229,12 @@ public async Task Integration_FullPipeline_FileReadParseSubstituteWrite()
229229
var parser = new MarkdownParser();
230230
var parseResult = parser.ParseContent(readResult.Value);
231231
Assert.True(parseResult.Success);
232-
232+
Assert.NotNull(parseResult.Value?.Variables);
233+
233234
// 3. Substitute variables
234235
var substituteResult = VariableSubstitutor.Substitute(parseResult.Value.Content, parseResult.Value.Variables, args);
235236
Assert.True(substituteResult.Success);
237+
Assert.NotNull(substituteResult.Value);
236238

237239
// 4. Write result
238240
var writeResult = await FileHelper.WriteFileAsync(outputFile, substituteResult.Value);
@@ -270,7 +272,8 @@ public async Task Integration_FullPipeline_ErrorPropagation()
270272
var parser = new MarkdownParser();
271273
var parseResult = parser.ParseContent(readResult.Value);
272274
Assert.True(parseResult.Success);
273-
275+
Assert.NotNull(parseResult.Value?.Content);
276+
274277
// Substitute with empty variables (should fail)
275278
var emptyArgs = new Dictionary<string, object>();
276279
var substituteResult = VariableSubstitutor.Substitute(parseResult.Value.Content, parseResult.Value.Variables, emptyArgs);
@@ -304,9 +307,12 @@ public async Task Integration_FullPipeline_WithJsonOutput()
304307

305308
// Act: Pipeline ending with JSON output
306309
var readResult = await FileHelper.ReadFileAsync(inputFile);
310+
Assert.NotNull(readResult.Value);
307311
var parser = new MarkdownParser();
308-
var parseResult = parser.ParseContent(readResult.Value!);
309-
var schemaJson = SchemaGenerator.GenerateSchema(parseResult.Value!.Variables);
312+
313+
var parseResult = parser.ParseContent(readResult.Value);
314+
Assert.NotNull(parseResult.Value?.Variables);
315+
var schemaJson = SchemaGenerator.GenerateSchema(parseResult.Value.Variables);
310316

311317
// Wrap in command response
312318
var commandResult = JsonOutput.Success(new { schema = schemaJson });
@@ -331,7 +337,6 @@ public void Integration_ProcessingResult_BindChaining()
331337
var content = "# Hello {{USER_NAME}}";
332338

333339
// Create a ProcessingResult and chain operations
334-
var parseResult = ProcessingResult<string>.Ok(content);
335340
var extractedVars = VariableExtractor.ExtractVariables(content);
336341

337342
// Create variables from extracted names

‎tests/MDTool.Tests/Models/ProcessingResultTests.cs‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ public void Fail_WithSingleError_CreatesFailedResult()
5757
// Assert
5858
Assert.False(result.Success);
5959
Assert.Single(result.Errors);
60-
Assert.Equal(ErrorType.FileNotFound, result.Errors[0].Type);
60+
Assert.Equal(ErrorType.FileNotFound, result.Errors.First().Type);
6161
}
6262

6363
[Fact]
@@ -126,7 +126,7 @@ public void Map_PropagatesErrorsOnFailure()
126126
// Assert
127127
Assert.False(mapped.Success);
128128
Assert.Single(mapped.Errors);
129-
Assert.Equal(ErrorType.FileNotFound, mapped.Errors[0].Type);
129+
Assert.Equal(ErrorType.FileNotFound, mapped.Errors.First().Type);
130130
}
131131

132132
[Fact]
@@ -141,8 +141,8 @@ public void Map_CatchesExceptionsAndReturnsError()
141141
// Assert
142142
Assert.False(mapped.Success);
143143
Assert.Single(mapped.Errors);
144-
Assert.Equal(ErrorType.ProcessingError, mapped.Errors[0].Type);
145-
Assert.Contains("Test error", mapped.Errors[0].Description);
144+
Assert.Equal(ErrorType.ProcessingError, mapped.Errors.First().Type);
145+
Assert.Contains("Test error", mapped.Errors.First().Description);
146146
}
147147

148148
[Fact]
@@ -172,7 +172,7 @@ public void Bind_PropagatesErrorsFromFirstOperation()
172172
// Assert
173173
Assert.False(bound.Success);
174174
Assert.Single(bound.Errors);
175-
Assert.Equal(ErrorType.FileNotFound, bound.Errors[0].Type);
175+
Assert.Equal(ErrorType.FileNotFound, bound.Errors.First().Type);
176176
}
177177

178178
[Fact]
@@ -189,7 +189,7 @@ public void Bind_PropagatesErrorsFromSecondOperation()
189189
// Assert
190190
Assert.False(bound.Success);
191191
Assert.Single(bound.Errors);
192-
Assert.Contains("Failed in second operation", bound.Errors[0].Description);
192+
Assert.Contains("Failed in second operation", bound.Errors.First().Description);
193193
}
194194

195195
[Fact]
@@ -392,7 +392,7 @@ public void ComplexChaining_ErrorPropagation()
392392
// Assert
393393
Assert.False(final.Success);
394394
Assert.Single(final.Errors);
395-
Assert.Contains("Mid-chain error", final.Errors[0].Description);
395+
Assert.Contains("Mid-chain error", final.Errors.First().Description);
396396
}
397397

398398
#endregion

‎tests/MDTool.Tests/Utilities/FileHelperTests.cs‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ public async Task ReadFileAsync_NonExistentFile_ReturnsFileNotFoundError()
5959
// Assert
6060
Assert.False(result.Success);
6161
Assert.Single(result.Errors);
62-
Assert.Equal(ErrorType.FileNotFound, result.Errors[0].Type);
62+
Assert.Equal(ErrorType.FileNotFound, result.Errors.First().Type);
6363
}
6464

6565
[Fact]
@@ -71,7 +71,7 @@ public async Task ReadFileAsync_NullPath_ReturnsInvalidPathError()
7171
// Assert
7272
Assert.False(result.Success);
7373
Assert.Single(result.Errors);
74-
Assert.Equal(ErrorType.InvalidPath, result.Errors[0].Type);
74+
Assert.Equal(ErrorType.InvalidPath, result.Errors.First().Type);
7575
}
7676

7777
[Fact]
@@ -83,7 +83,7 @@ public async Task ReadFileAsync_EmptyPath_ReturnsInvalidPathError()
8383
// Assert
8484
Assert.False(result.Success);
8585
Assert.Single(result.Errors);
86-
Assert.Equal(ErrorType.InvalidPath, result.Errors[0].Type);
86+
Assert.Equal(ErrorType.InvalidPath, result.Errors.First().Type);
8787
}
8888

8989
[Fact]
@@ -95,7 +95,7 @@ public async Task ReadFileAsync_WhitespacePath_ReturnsInvalidPathError()
9595
// Assert
9696
Assert.False(result.Success);
9797
Assert.Single(result.Errors);
98-
Assert.Equal(ErrorType.InvalidPath, result.Errors[0].Type);
98+
Assert.Equal(ErrorType.InvalidPath, result.Errors.First().Type);
9999
}
100100

101101
[Fact]
@@ -161,7 +161,7 @@ public async Task ReadFileAsync_FileExceedsSizeLimit_ReturnsFileSizeExceededErro
161161
// Assert
162162
Assert.False(result.Success);
163163
Assert.Single(result.Errors);
164-
Assert.Equal(ErrorType.FileSizeExceeded, result.Errors[0].Type);
164+
Assert.Equal(ErrorType.FileSizeExceeded, result.Errors.First().Type);
165165
}
166166

167167
#endregion
@@ -198,7 +198,7 @@ public async Task WriteFileAsync_ExistingFileWithoutForce_ReturnsFileExistsError
198198
// Assert
199199
Assert.False(result.Success);
200200
Assert.Single(result.Errors);
201-
Assert.Equal(ErrorType.FileExists, result.Errors[0].Type);
201+
Assert.Equal(ErrorType.FileExists, result.Errors.First().Type);
202202

203203
// Verify original content is unchanged
204204
var actualContent = await File.ReadAllTextAsync(filePath);
@@ -270,7 +270,7 @@ public async Task WriteFileAsync_NullPath_ReturnsInvalidPathError()
270270
// Assert
271271
Assert.False(result.Success);
272272
Assert.Single(result.Errors);
273-
Assert.Equal(ErrorType.InvalidPath, result.Errors[0].Type);
273+
Assert.Equal(ErrorType.InvalidPath, result.Errors.First().Type);
274274
}
275275

276276
[Fact]
@@ -282,7 +282,7 @@ public async Task WriteFileAsync_EmptyPath_ReturnsInvalidPathError()
282282
// Assert
283283
Assert.False(result.Success);
284284
Assert.Single(result.Errors);
285-
Assert.Equal(ErrorType.InvalidPath, result.Errors[0].Type);
285+
Assert.Equal(ErrorType.InvalidPath, result.Errors.First().Type);
286286
}
287287

288288
[Fact]
@@ -297,7 +297,7 @@ public async Task WriteFileAsync_NullContent_ReturnsProcessingError()
297297
// Assert
298298
Assert.False(result.Success);
299299
Assert.Single(result.Errors);
300-
Assert.Equal(ErrorType.ProcessingError, result.Errors[0].Type);
300+
Assert.Equal(ErrorType.ProcessingError, result.Errors.First().Type);
301301
}
302302

303303
[Fact]
@@ -356,7 +356,7 @@ public void ValidatePath_NullPath_ReturnsInvalidPathError()
356356
// Assert
357357
Assert.False(result.Success);
358358
Assert.Single(result.Errors);
359-
Assert.Equal(ErrorType.InvalidPath, result.Errors[0].Type);
359+
Assert.Equal(ErrorType.InvalidPath, result.Errors.First().Type);
360360
}
361361

362362
[Fact]
@@ -368,7 +368,7 @@ public void ValidatePath_EmptyPath_ReturnsInvalidPathError()
368368
// Assert
369369
Assert.False(result.Success);
370370
Assert.Single(result.Errors);
371-
Assert.Equal(ErrorType.InvalidPath, result.Errors[0].Type);
371+
Assert.Equal(ErrorType.InvalidPath, result.Errors.First().Type);
372372
}
373373

374374
[Fact]
@@ -380,7 +380,7 @@ public void ValidatePath_WhitespacePath_ReturnsInvalidPathError()
380380
// Assert
381381
Assert.False(result.Success);
382382
Assert.Single(result.Errors);
383-
Assert.Equal(ErrorType.InvalidPath, result.Errors[0].Type);
383+
Assert.Equal(ErrorType.InvalidPath, result.Errors.First().Type);
384384
}
385385

386386
[Fact]
@@ -395,7 +395,7 @@ public void ValidatePath_WithStrictMode_DotsInPath_ReturnsPathTraversalError()
395395
// Assert
396396
Assert.False(result.Success);
397397
Assert.Single(result.Errors);
398-
Assert.Equal(ErrorType.PathTraversalAttempt, result.Errors[0].Type);
398+
Assert.Equal(ErrorType.PathTraversalAttempt, result.Errors.First().Type);
399399
}
400400

401401
[Fact]
@@ -410,7 +410,7 @@ public void ValidatePath_WithStrictMode_TildeInPath_ReturnsPathTraversalError()
410410
// Assert
411411
Assert.False(result.Success);
412412
Assert.Single(result.Errors);
413-
Assert.Equal(ErrorType.PathTraversalAttempt, result.Errors[0].Type);
413+
Assert.Equal(ErrorType.PathTraversalAttempt, result.Errors.First().Type);
414414
}
415415

416416
[Fact]
@@ -473,7 +473,7 @@ public async Task CheckFileSize_ExceedsLimit_ReturnsFileSizeExceededError()
473473
// Assert
474474
Assert.False(result.Success);
475475
Assert.Single(result.Errors);
476-
Assert.Equal(ErrorType.FileSizeExceeded, result.Errors[0].Type);
476+
Assert.Equal(ErrorType.FileSizeExceeded, result.Errors.First().Type);
477477
}
478478

479479
[Fact]
@@ -503,7 +503,7 @@ public void CheckFileSize_NonExistentFile_ReturnsFileNotFoundError()
503503
// Assert
504504
Assert.False(result.Success);
505505
Assert.Single(result.Errors);
506-
Assert.Equal(ErrorType.FileNotFound, result.Errors[0].Type);
506+
Assert.Equal(ErrorType.FileNotFound, result.Errors.First().Type);
507507
}
508508

509509
[Fact]
@@ -535,7 +535,7 @@ public async Task CheckFileSize_CustomLimit_Exceeds()
535535
// Assert
536536
Assert.False(result.Success);
537537
Assert.Single(result.Errors);
538-
Assert.Equal(ErrorType.FileSizeExceeded, result.Errors[0].Type);
538+
Assert.Equal(ErrorType.FileSizeExceeded, result.Errors.First().Type);
539539
}
540540

541541
[Fact]

0 commit comments

Comments
 (0)