Add collector for Reigate and Banstead Borough Council#174
Add collector for Reigate and Banstead Borough Council#174moley-bot[bot] wants to merge 3 commits intomainfrom
Conversation
Closes #141 Generated with Codex CLI by Moley-Bot
moley-bot
bot
added
the
new collector
Formatted by Moley-Bot
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR adds a new collector for Reigate and Banstead Borough Council. The implementation is very complex, with the collector class exceeding 800 lines, which goes against the style guide's recommendation for simplicity. The main issue is the large, duplicated request bodies which should be refactored into a shared helper method. There are also several minor style guide violations, such as missing trailing commas and inconsistent use of modern C# features like collection expressions. Additionally, there's an inconsistency in metadata handling for derivable values, which should be addressed by deriving them locally rather than storing them in metadata. While the collector appears to be functional, addressing these points will significantly improve its maintainability.
| /// <summary> | ||
| /// Builds the request body for the property details lookup. | ||
| /// </summary> | ||
| private static string BuildPropertyDetailsBody(Dictionary<string, string> metadata) |
There was a problem hiding this comment.
This collector is over 800 lines long, which significantly exceeds the 200-400 line recommendation in the style guide and indicates excessive complexity. A primary reason for this is the large amount of duplicated code between the BuildPropertyDetailsBody and BuildScheduleRequestBody methods. The JSON structure for formValues is almost identical in both.
To adhere to the DRY principle and improve maintainability, please extract the common JSON object structure into a dedicated helper method. This will significantly reduce the line count and make the collector easier to understand and maintain.
| "lengthOfPostcode": { "value": "{{postcode.Length}}" }, | ||
| "lengthMinus2": { "value": "{{postcode.Length - 2}}" }, |
There was a problem hiding this comment.
There's an inconsistency in how lengthOfPostcode and lengthMinus2 are handled. These values are stored in the metadata dictionary in GetBinDays (step 1), but this method recalculates them inline from the postcode variable. The BuildScheduleRequestBody method, however, uses the values from metadata. According to repository conventions, values that can be easily derived, such as these, should not be stored in metadata. To resolve this inconsistency and adhere to best practices, please remove lengthOfPostcode and lengthMinus2 from the metadata dictionary and ensure both BuildScheduleRequestBody and this method derive these values locally from the postcode variable.
References
- Only store metadata that is necessary for subsequent requests. Avoid storing values that can be easily derived. (link)
- In collectors, prioritize clarity by keeping the request flow for each method (e.g.,
GetAddresses,GetBinDays) self-contained, even if it introduces some code duplication for initial requests.
| Keys = [ "Garden waste" ], | ||
| }, |
There was a problem hiding this comment.
According to the style guide, all multi-line initializers should have trailing commas. This helps with version control diffs and makes adding new items easier.
Keys = [ "Garden waste" ],
},References
- Always use trailing commas in multi-line initializers to make future diffs cleaner and reduce merge conflicts. (link)
| { "cookie", cookies }, | ||
| { "user-agent", Constants.UserAgent }, |
There was a problem hiding this comment.
According to the style guide, all multi-line initializers should have trailing commas. This is missing here and in other header initializations throughout the file.
{ "cookie", cookies },
{ "user-agent", Constants.UserAgent },References
- Always use trailing commas in multi-line initializers to make future diffs cleaner and reduce merge conflicts. (link)
| var dateString = section.Groups["date"].Value.Trim(); | ||
| var binsContent = section.Groups["content"].Value; | ||
|
|
||
| var binNames = BinNameRegex().Matches(binsContent)!.Select(match => match.Groups["bin"].Value.Trim()).ToList(); |
There was a problem hiding this comment.
The style guide prefers using C# 12 collection expressions ([..]) over .ToList() for creating lists. Please update this line to use a collection expression for consistency.
var binNames = [.. BinNameRegex().Matches(binsContent)!.Select(match => match.Groups["bin"].Value.Trim())];References
- Use collection expressions (
[.. collection]) instead of.ToList().AsReadOnly()or.ToList()for creating collections, as it is more concise modern C# 12 syntax. (link)
| DateTimeStyles.None | ||
| ); | ||
|
|
||
| var matchedBins = binNames.SelectMany(binName => ProcessingUtilities.GetMatchingBins(_binTypes, binName)).DistinctBy(bin => bin.Name).ToList(); |
There was a problem hiding this comment.
The style guide prefers using C# 12 collection expressions ([..]) over .ToList() for creating lists. Please update this line to use a collection expression for consistency.
var matchedBins = [.. binNames.SelectMany(binName => ProcessingUtilities.GetMatchingBins(_binTypes, binName)).DistinctBy(bin => bin.Name)];References
- Use collection expressions (
[.. collection]) instead of.ToList().AsReadOnly()or.ToList()for creating collections, as it is more concise modern C# 12 syntax. (link)
|
@claude Please fix all unresolved PR comments, making sure to test after each change made. Please include the test output in your final comment update. |
|
Claude finished @BadgerHobbs's task in 19m 59s —— View job Fixing PR Review CommentsAll unresolved review comments have been addressed. Here's a summary:
Note: Trailing comma issues (lines 66, 156) were already resolved by the Changes Made
Collection expressions: Changed Net result: 833 → 812 lines (-21 lines, with the helper itself adding ~40 lines, meaning ~61 lines of duplication removed). Test Output| |
- Extract 27 common address form fields into BuildAddressFormValues helper to eliminate duplication between BuildPropertyDetailsBody and BuildScheduleRequestBody - Remove lengthOfPostcode and lengthMinusTwo from metadata dictionary and derive them locally from postcode.Length in both request bodies - Use collection expressions with explicit types instead of .ToList() for binNames and matchedBins Co-authored-by: Andrew Riggs <BadgerHobbs@users.noreply.github.com>
1 participant
Summary
This PR adds a new bin collection data collector for Reigate and Banstead Borough Council.
ICollectorinterfaceCloses #141
Test Summary
Bin Collections Page Screenshot
Generated automatically by Moley-Bot using Codex CLI