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.

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

Code Review: Refactor code with new method #1

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

kattsky
Copy link

@kattsky kattsky commented Mar 23, 2024

No description provided.

@kattsky
Copy link
Author

kattsky commented Mar 23, 2024

Hi Ravi, as the Issue tab is unavailable in the repo, I've decided to add comments in your CONTRIBUTING.md file for the code review.
Then I forked your repo and raised a PR as a workaround.

Great work with addressing the problem in your code.
Maybe for a little bit of enhancement to make the code more readable and robust, you could consider introducing a method called DeserializeNonNullValues(JsonData jsonData) to extract non-null values from JSON data. Then returns a 'Student' object that has just non-null values.

This will help simplify the logic in Main method. Plus, it conforms to the principle of encapsulation.
Let me know your thoughts. Great work, Ravi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants