Conversation
Closes #68 Generated with Codex CLI by Moley-Bot
Summary of ChangesHello @BadgerHobbs, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new data collector specifically designed for Kirklees Council, enabling the system to programmatically retrieve bin collection schedules. The implementation involves a multi-step process of client-side requests to interact with the council's web service, first to identify addresses based on a postcode, and then to fetch detailed bin collection dates for a selected property. This enhancement expands the system's coverage to a new local authority, improving its utility for users in the Kirklees area. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Formatted by Moley-Bot
![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 introduces a new collector for Kirklees Council, which involves a complex multi-step process to retrieve bin collection data. The implementation correctly follows the project's pattern for handling client-side requests and state management. The integration test is also well-formed and adheres to the provided template.
My review identifies several minor areas for improvement, primarily related to adherence to the repository's C# style guide. These include consistently using trailing commas, avoiding inline returns for complex objects, and extracting hardcoded values like URLs and date ranges into constants for better maintainability. I've also noted a small code formatting issue. Addressing these points will enhance code quality and consistency with the rest of the project.
| { "fromDate", DateTime.UtcNow.AddDays(-28).ToString("dd/MM/yyyy", CultureInfo.InvariantCulture) }, | ||
| { "toDate", DateTime.UtcNow.AddDays(28).ToString("dd/MM/yyyy", CultureInfo.InvariantCulture) }, |
There was a problem hiding this comment.
The number 28 is used to define the date range for fetching bin collections. To improve readability and maintainability, consider extracting this magic number into a named private const int at the top of the class.
For example:
private const int _dateRangeDays = 28;
// ... later in the code
{ "fromDate", DateTime.UtcNow.AddDays(-_dateRangeDays).ToString(...) },
{ "toDate", DateTime.UtcNow.AddDays(_dateRangeDays).ToString(...) },| private static Dictionary<string, object> BuildAddressSearchBody(string postcode) | ||
| { | ||
| return new Dictionary<string, object> | ||
| { | ||
| { | ||
| "formValues", | ||
| new Dictionary<string, object> | ||
| { | ||
| { | ||
| "Section 1", | ||
| new Dictionary<string, object> | ||
| { | ||
| { "searchForAddress", CreateField("searchForAddress", "yes", true, "radio") }, | ||
| { "Postcode", CreateField("Postcode", postcode, true) }, | ||
| { "postcode", CreateField("postcode", string.Empty, true) }, | ||
| { "house", CreateField("house", string.Empty, true) }, | ||
| { "flat", CreateField("flat", string.Empty, true) }, | ||
| { "street", CreateField("street", string.Empty, true) }, | ||
| { "town", CreateField("town", string.Empty, true) }, | ||
| { "fullAddress", CreateField("fullAddress", string.Empty, true) }, | ||
| } | ||
| }, | ||
| } | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
This method uses an inline return new Dictionary..., which violates the style guide (rule #337) that requires using a separate variable declaration before returning. This improves readability and makes debugging easier.
This pattern is also repeated in the BuildSearchSection and CreateField helper methods.
private static Dictionary<string, object> BuildAddressSearchBody(string postcode)
{
var body = new Dictionary<string, object>
{
{
"formValues",
new Dictionary<string, object>
{
{
"Section 1",
new Dictionary<string, object>
{
{ "searchForAddress", CreateField("searchForAddress", "yes", true, "radio") },
{ "Postcode", CreateField("Postcode", postcode, true) },
{ "postcode", CreateField("postcode", string.Empty, true) },
{ "house", CreateField("house", string.Empty, true) },
{ "flat", CreateField("flat", string.Empty, true) },
{ "street", CreateField("street", string.Empty, true) },
{ "town", CreateField("town", string.Empty, true) },
{ "fullAddress", CreateField("fullAddress", string.Empty, true) },
}
},
}
},
};
return body;
}References
- Always use separate variable declarations, not inline returns. (link)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new collector for Kirklees Council, which is a complex, multi-step implementation. The code is well-structured and successfully implements the required logic. My review focuses on improving adherence to the repository's C# style guide, particularly concerning the construction of JSON request bodies, the implementation of helper methods, and consistent use of modern C# syntax. Refactoring these areas will significantly improve the code's readability and maintainability.
| var requestBodyObject = BuildAddressSearchBody(postcode); | ||
| var requestBody = JsonSerializer.Serialize(requestBodyObject); |
There was a problem hiding this comment.
This approach of building the request body using a helper method (BuildAddressSearchBody) and JsonSerializer.Serialize violates a few style guide rules:
- Single-Use Helper:
BuildAddressSearchBodyis only called once, and the style guide (rule #208) recommends inlining single-use helpers. - JSON Payload Construction: The guide (rule #316) strongly prefers using raw string literals (
$$"""..."""`) over nested dictionaries for building JSON payloads, as they are more readable.
I suggest inlining the logic and converting it to a raw string literal. This also allows you to create a minimal payload, as recommended by rule #228.
var requestBody = $$`"`{
"formValues": {
"Section 1": {
"Postcode": { "name": "Postcode", "value": "{{postcode}}", "isMandatory": true },
"searchForAddress": { "name": "searchForAddress", "value": "yes", "isMandatory": true, "type": "radio" }
}
}
}`"`;| private static Dictionary<string, object> CreateField(string name, string value, bool isMandatory, string type = "text", string? valueLabel = null) | ||
| { | ||
| return new Dictionary<string, object> | ||
| { | ||
| { "name", name }, | ||
| { "type", type }, | ||
| { "id", string.Empty }, | ||
| { "value_changed", true }, | ||
| { "section_id", string.Empty }, | ||
| { "label", name }, | ||
| { "value_label", valueLabel ?? string.Empty }, | ||
| { "hasOther", false }, | ||
| { "value", value }, | ||
| { "path", string.Empty }, | ||
| { "valid", true }, | ||
| { "totals", string.Empty }, | ||
| { "suffix", string.Empty }, | ||
| { "prefix", string.Empty }, | ||
| { "summary", string.Empty }, | ||
| { "hidden", false }, | ||
| { "_hidden", false }, | ||
| { "isSummary", false }, | ||
| { "staticMap", false }, | ||
| { "isMandatory", isMandatory }, | ||
| { "isRepeatable", false }, | ||
| { "currencyPrefix", string.Empty }, | ||
| { "decimalPlaces", string.Empty }, | ||
| { "hash", string.Empty }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
This CreateField helper and the other request-building helpers in this file violate several style guide rules:
- Bloated Payloads:
CreateFieldadds many fields with default or empty values (e.g.,id,path,summary). This violates the minimal payload principle (rule #228). - Nested Dictionaries: The helpers use nested
Dictionary<string, object>to build JSON. The style guide (rule #316) strongly prefers raw string literals ($$"""..."""`) for readability. - Single-Use Helpers:
BuildAddressSearchBodyandGetBinTypeServiceare only used once and should be inlined (rule #208). - Missing Documentation: None of the private helper methods have the required XML documentation (rule #222).
I recommend a significant refactor to remove CreateField and the single-use helpers. The request bodies should be built directly where they are used with minimal raw string literals.
References
- Remove empty/null/default/false fields: Don't include fields in request bodies that have empty strings, null values, default values, or false booleans unless the API strictly requires them. (link)
- Building JSON payloads: Use raw string literals with interpolation ($$"""...""") instead of nested Dictionary structures for better readability. (link)
- Create helpers only for duplication (2-3+ uses). Don't extract single-use methods. (link)
- Always add XML documentation (
/// <summary>) for helper methods. (link)
| var metadata = new ClientSideOptions | ||
| { | ||
| Metadata = | ||
| { | ||
| { "sid", clientSideResponse.Options.Metadata["sid"] }, | ||
| { "cookies", clientSideResponse.Options.Metadata["cookies"] }, | ||
| { "govDeliveryCategory", clientSideResponse.Options.Metadata["govDeliveryCategory"] }, | ||
| { "fromDate", clientSideResponse.Options.Metadata["fromDate"] }, | ||
| { "toDate", clientSideResponse.Options.Metadata["toDate"] }, | ||
| { "binData", binData }, | ||
| { "binIndex", "0" }, | ||
| { "bins", JsonSerializer.Serialize(bins) }, | ||
| { "binDays", "[]" }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
A new ClientSideOptions object is created here, and several metadata values are copied from the incoming clientSideResponse. This is redundant and inconsistent with the logic in the next step (RequestId == 4), which modifies the existing options. To simplify, you can add the new metadata keys directly to clientSideResponse.Options.Metadata.
clientSideResponse.Options.Metadata.Add("binData", binData);
clientSideResponse.Options.Metadata.Add("binIndex", "0");
clientSideResponse.Options.Metadata.Add("bins", JsonSerializer.Serialize(bins));
clientSideResponse.Options.Metadata.Add("binDays", "[]");
var metadata = clientSideResponse.Options;| { "Postcode", CreateField("Postcode", address.Postcode ?? string.Empty, true) }, | ||
| { "List", CreateField("List", address.Uid ?? string.Empty, true, "select", address.Property) }, | ||
| { "House", CreateField("House", house, true) }, | ||
| { "Street", CreateField("Street", address.Street ?? string.Empty, true) }, | ||
| { "Town", CreateField("Town", address.Town ?? string.Empty, true) }, | ||
| { "UPRN", CreateField("UPRN", address.Uid ?? string.Empty, true) }, |
There was a problem hiding this comment.
The null-coalescing operator (?? string.Empty) is used here for address properties that are expected to exist. The repository style guide (rule #391) recommends using the null-forgiving operator (!) to ensure the collector fails fast if required data is missing.
{ "Postcode", CreateField("Postcode", address.Postcode!, true) },
{ "List", CreateField("List", address.Uid!, true, "select", address.Property) },
{ "House", CreateField("House", house, true) },
{ "Street", CreateField("Street", address.Street!, true) },
{ "Town", CreateField("Town", address.Town!, true) },
{ "UPRN", CreateField("UPRN", address.Uid!, true) },References
- Use null-forgiving operator
!when values are required: When a value must be non-null for the collector to work properly, use the null-forgiving operator instead of defensive null checks. (link)
|
@claude Address/resolve all outstanding PR comments. Test/validate your changes, and the code against the style guide. |
|
Claude finished @BadgerHobbs's task in 6m 19s —— View job Addressing PR Review Comments ✅Successfully refactored the KirkleesCouncil collector to address all PR review comments. Summary of ChangesAll PR review comments have been addressed:
Impact
Commit Details
The CI pipeline will automatically validate the build and run tests. All style guide requirements have been followed. --- | |
- Extract hardcoded URLs and configuration to constants - Convert nested dictionaries to raw string literals for JSON payloads - Remove single-use helper methods (BuildAddressSearchBody, BuildSearchSection, CreateField, GetBinTypeService) - Add trailing commas to all multi-line initializers - Use null-forgiving operator for required address fields - Use HashSet for O(1) duplicate checking instead of List.Any() - Simplify ClientSideOptions handling in RequestId 3 - Add XML documentation to BuildScheduleRequest helper Co-authored-by: Andrew Riggs <BadgerHobbs@users.noreply.github.com>
1 participant
Summary
This PR adds a new bin collection data collector for Kirklees Council.
ICollectorinterfaceCloses #68
Test Summary
Generated automatically by Moley-Bot using Codex CLI