-
Notifications
You must be signed in to change notification settings - Fork 16
Refactor SunKit's Sun #58
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.
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
Open
Sireikis
wants to merge
91
commits into
SunKit-Swift:main
Choose a base branch
from
Sireikis:story/refactor-sun
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…el concept organization.
…izonCoordinates function.
…SunHorizonCoordinates.
…orizonCoordinates in SunCoordinates.
…bject. Now using the setCoordinates method on SunCoordinates.
Hey, thanks for your contribution. Looks good to me, and I think that a cleanup was much needed. The other maintainer will review the changes too and possibly allow a merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Disclaimer
This is an exercise in refactoring as defined by Martin Fowler--changing the internal structure of a program without changing behavior. No tests have been added or changed. No public API's should have been changed. I took care to make sure all tests pass and code compiles before every commit. Changes primarily consist of making code style more consistent and, as mentioned before, transforming the internal structure of code without altering behavior. Whether or not behavior has actually been changed however, depends entirely on the strength of the tests.
I'm aware refactoring is only really useful for code that changes, but since I did this for fun I may as well share my work, it may end up being useful.
Goal
The goal of this refactor was primarily to impose a more consistent style across SunKit and to make the Sun struct more readable. Care was taken to not alter any public APIs.
Note that there are files that have not been refactored: Angle, DMS, HMS, SunElevationEvents, and Utils.
Changes
-Style was made consistent by normalizing vertical and horizontal spacing. Spaces between types, functions, and operators were added or removed depending on context. Function names were given a consistent naming scheme when possible. Generally these changes should follow the swift language doc guidelines. Redundant comments (using Martin Fowler's definition) were removed when the code was self explanatory.
-Methods that are extensions on types were moved to appropriately named folders following the + naming convention.
-Mutations to EclipticCoordinates, EquatorialCoordinates, and HorizonCoordinates were encapsulated by the SunCoordinates struct.
Potential Future work
-There is more work to be done in Sun, such as encapsulating the Day Events and Month Events logic. That could be done in future PRs depending on the reception to this one.
-There is more refactoring to do in the various Coordinates structs, particularly the conversion code and the model mutation in EquatorialCoordinates. However, altering the way the EquatorialCoordinates mutates the model would require changes to the public API.
-Refactor Angle, DMS, HMS, SunElevationEvents, and Utils using the same process as in this PR.