Add collector for Broxtowe Borough Council#130
Conversation
Closes #107 Generated with Codex CLI by Moley-Bot
Formatted by Moley-Bot
| }, | ||
| ]; | ||
|
|
||
| private const string _formUrl = "https://selfservice.broxtowe.gov.uk/renderform.aspx?t=217&k=9D2EF214E144EE796430597FB475C3892C43C528"; |
There was a problem hiding this comment.
private fields should have summary docstrings, and ones with only one reference removed.
| // Prepare client-side request for postcode search | ||
| else if (clientSideResponse.RequestId == 1) | ||
| { | ||
| clientSideResponse.Headers.TryGetValue("set-cookie", out var setCookieHeader); |
There was a problem hiding this comment.
should expect the cookie if required, not try get.
| { "__VIEWSTATE", viewState }, | ||
| { "__VIEWSTATEGENERATOR", viewStateGenerator }, | ||
| { "__EVENTVALIDATION", eventValidation }, | ||
| { "ctl00$ContentPlaceHolder1$txtPositionLL", string.Empty }, |
There was a problem hiding this comment.
Can probably remove null/empty/default values.
| return getBinDaysResponse; | ||
| } | ||
| // Prepare client-side request for selecting the address | ||
| else if (clientSideResponse.RequestId == 2) |
There was a problem hiding this comment.
Why are we getting the address twice? Should already have the data we need in the address object already.
| } | ||
| } | ||
|
|
||
| throw new InvalidOperationException($"{fieldName} not found."); |
There was a problem hiding this comment.
If we're adding errors, could be more specific.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
/gemini |
|
It looks like you're trying to invoke a command, but you haven't specified which one. Please use
|
|
/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 pull request adds a new collector for Broxtowe Borough Council. The implementation is quite complex due to the multi-step ASP.NET Web Forms process it needs to replicate. The code is well-structured and follows most of the project's style guide.
My review includes a few suggestions to improve adherence to the style guide and maintainability:
- Style Guide: Adding missing trailing commas in collection initializers and inlining a constant that is only used once.
- Maintainability: There's significant code duplication between
GetAddressesandGetBinDaysfor the first couple of steps. I've suggested extracting this logic into private helper methods to follow the DRY principle. - Refactoring: A small suggestion to make the
GetHiddenFieldhelper method more concise using LINQ.
The accompanying integration test is well-written and follows the project's template. Overall, this is a solid contribution. Addressing the feedback will make the code cleaner and easier to maintain.
| new() | ||
| { | ||
| Name = "Mixed Dry Recycling", | ||
| Colour = BinColour.Green, | ||
| Keys = [ "GREEN 240L" ], | ||
| }, | ||
| new() | ||
| { | ||
| Name = "Glass Recycling", | ||
| Colour = BinColour.Green, | ||
| Keys = [ "GLASS BAG" ], | ||
| Type = BinType.Bag, | ||
| }, | ||
| new() | ||
| { | ||
| Name = "Garden Waste", | ||
| Colour = BinColour.Brown, | ||
| Keys = [ "BROWN 240L" ], | ||
| }, | ||
| new() | ||
| { | ||
| Name = "General Waste", | ||
| Colour = BinColour.Black, | ||
| Keys = [ "BLACK 240L" ], | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
The style guide requires trailing commas in multi-line initializers to make future diffs cleaner. Several initializers in this file are missing them, including the Keys collections, the last element of the _binTypes collection, and various Headers and formData dictionaries in the methods. Please add them for consistency.
new()
{
Name = "Mixed Dry Recycling",
Colour = BinColour.Green,
Keys = [ "GREEN 240L", ],
},
new()
{
Name = "Glass Recycling",
Colour = BinColour.Green,
Keys = [ "GLASS BAG", ],
Type = BinType.Bag,
},
new()
{
Name = "Garden Waste",
Colour = BinColour.Brown,
Keys = [ "BROWN 240L", ],
},
new()
{
Name = "General Waste",
Colour = BinColour.Black,
Keys = [ "BLACK 240L", ],
},References
- Always use trailing commas in multi-line initializers. This makes future diffs cleaner and reduces merge conflicts. (link)
| private const string _scriptManagerTarget = "ctl00$ContentPlaceHolder1$APUP_5683"; | ||
| private const string _searchEventTarget = "ctl00$ContentPlaceHolder1$FF5683BTN"; | ||
| private const string _addressEventTarget = "ctl00$ContentPlaceHolder1$FF5683DDL"; | ||
| private const string _submitEventTarget = "ctl00$ContentPlaceHolder1$btnSubmit"; |
There was a problem hiding this comment.
The constant _submitEventTarget is only used once in the file (on line 343). According to the style guide, values used only once should be inlined to reduce clutter. Please remove this constant and use the string literal directly where it's needed.
References
- Constants should only be created for values that are used two or more times. Single-use values should be inlined directly at their point of use to avoid unnecessary declarations. (link)
1 participant
Summary
This PR adds a new bin collection data collector for Broxtowe Borough Council.
ICollectorinterfaceCloses #107
Test Summary
Generated automatically by Moley-Bot using Codex CLI