Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

C# Source Modernization initiative #430

Merged
merged 26 commits into from
Jul 23, 2019
Merged

C# Source Modernization initiative #430

merged 26 commits into from
Jul 23, 2019

Conversation

kvakvs
Copy link
Collaborator

@kvakvs kvakvs commented Jul 16, 2019 •
edited

NOTE

This PR does not add, modify or remove features. This PR features code formatting, logging and quality of life changes to the developer experience.

Changes

This PR only includes files from CSUtil.Commons and TLM/Custom

For every file in TLM:

  • Untabify
  • Optimize imports (for FullDebug)
  • Format and wrap too long lines, or where StyleCop shows a warning
  • Apply minor Resharper suggestions and refactor
    • every var becomes explicit type (solves Enforce stating explicit types #436, debatable)
    • var Xxx used in out Xxx args, are inlined into out var Xxx (latest C#)
    • break deep nested if's where possible make the code more flat
    • extract huge code blocks into separate functions
  • Get rid of magic constants where possible
  • Mark redirected functions as [UsedImplicitly] to get rid of the warning
  • #if DEBUG if (condition) Log._Debug #endif changed to something shorter (solves Improve code readability by simplifying debug logging #349)

@kvakvs kvakvs added code cleanup Refactor code, remove old code, improve maintainability DO NOT MERGE YET Don't merge this PR, even if approved, until further notice labels Jul 16, 2019
@kvakvs kvakvs self-assigned this Jul 16, 2019
@originalfoo originalfoo added this to the 11.0 milestone Jul 16, 2019
@originalfoo originalfoo mentioned this pull request Jul 18, 2019
Closed
originalfoo
originalfoo reviewed Jul 18, 2019
View reviewed changes
TLM/.editorconfig Outdated Show resolved Hide resolved
originalfoo
originalfoo reviewed Jul 18, 2019
View reviewed changes
TLM/.editorconfig Outdated Show resolved Hide resolved
@kvakvs kvakvs mentioned this pull request Jul 19, 2019
Closed
@kvakvs kvakvs added under-review A pull request has been created and is currently being reviewed and removed DO NOT MERGE YET Don't merge this PR, even if approved, until further notice labels Jul 19, 2019
krzychu124
krzychu124 approved these changes Jul 19, 2019
View reviewed changes
Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Tested in-game, no issues.

@krzychu124
Copy link
Member

Since we are reviewing this PR would be nice to change it's state to 'Ready for review' 😉

@kvakvs kvakvs marked this pull request as ready for review July 19, 2019 22:52
VictorPhilipp
VictorPhilipp previously requested changes Jul 20, 2019
View reviewed changes
TLM/TLM/Custom/AI/CustomAmbulanceAI.cs Outdated Show resolved Hide resolved
TLM/TLM/Custom/AI/CustomBuildingAI.cs Show resolved Hide resolved
TLM/TLM/Custom/AI/CustomBusAI.cs Outdated Show resolved Hide resolved
TLM/CSUtil.Commons/Log.cs Outdated Show resolved Hide resolved
TLM/CSUtil.Commons/Log.cs Outdated Show resolved Hide resolved
TLM/TLM/Custom/PathFinding/CustomPathFind_Old.cs Outdated Show resolved Hide resolved
TLM/TLM/Geometry/Impl/SegmentEndId.cs Outdated Show resolved Hide resolved
TLM/TLM/Geometry/Impl/SegmentEndId.cs Outdated Show resolved Hide resolved
TLM/TLM/Manager/AbstractGeometryObservingManager.cs Outdated Show resolved Hide resolved
TLM/TLM/Manager/Impl/AdvancedParkingManager.cs Show resolved Hide resolved
originalfoo
originalfoo approved these changes Jul 22, 2019
View reviewed changes
Copy link
Member

@originalfoo originalfoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@kvakvs kvakvs dismissed VictorPhilipp’s stale review July 23, 2019 14:17

I have addressed and responded to all notes except the multiline comments

@kvakvs kvakvs merged commit 94b27fe into CitiesSkylinesMods:master Jul 23, 2019
@kvakvs kvakvs mentioned this pull request Jul 23, 2019
Merged
2 tasks
@originalfoo originalfoo added technical Tasks that need to be performed in order to improve quality and maintainability and removed under-review A pull request has been created and is currently being reviewed labels Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dymanoid dymanoid dymanoid left review comments

@originalfoo originalfoo originalfoo approved these changes

@krzychu124 krzychu124 krzychu124 approved these changes

@VictorPhilipp VictorPhilipp VictorPhilipp left review comments

Assignees

@kvakvs kvakvs

Labels
code cleanup Refactor code, remove old code, improve maintainability Harmony technical Tasks that need to be performed in order to improve quality and maintainability
Projects
None yet
Milestone
11.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kvakvs @krzychu124 @originalfoo @VictorPhilipp @dymanoid