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 clarity update #492

Merged
merged 6 commits into from
Jul 13, 2024
Merged

Code clarity update #492

merged 6 commits into from
Jul 13, 2024

Conversation

rkrishnasanka
Copy link
Contributor

The code currently is hard to work with because of the lack of documentation, the crazy number of features pack in and usage of shorthand variables.

This PR tries to improve the situation for the following scenarios:

  1. Renaming all struct fields to not have shorthand
  2. Replacing redundant variables
  3. Ensuring that all type structs are PascalCase rather than camelCase since it messes up exports and makes the code hard to read since there is no difference in the instance names and the typedef. This also has the advantage of making sure that all teh structs are available outside the package scope

Copy link

vercel bot commented Apr 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
graphjin ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 12, 2024 0:14am

@dosco
Copy link
Owner

dosco commented Apr 13, 2024

Sent you a couple thoughts on discord

@dosco
Copy link
Owner

dosco commented Apr 16, 2024

3. Ensuring that all type structs are PascalCase rather than camelCase since it messes up exports and makes the code hard to read since there is no difference in the instance names and the typedef. This also has the advantage of making sure that all teh structs are available outside the package scope

let me know your thoughts on this since "not exposing" the structs outside the package scope was the point of keeping them camelCase. The idea is that those structs etc are part of the internal API and should not be exposed as they would unnecessarily need to be included in the backward compatibility guarantee slowing down development and also bloating the api documentation.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ rkrishnasanka
❌ deepsource-io[bot]
You have signed the CLA already but the status is still pending? Let us recheck it.

Improving code clarity
I'm starting to make some changes to the code documentation here. I'm including a bit of refactoring to make sure that things are more readable.

- The code extensively uses the short variable names to the detriment of the readability. This compounds the problem of not having enough code documentation.

- While golang recommends short variable names, the intention is to avoid long nested function calls and to avoid diverting the attention from the main logic of the program. However, even the main logic variables have really short names making it almost impossible to read.

Incremental renaming of variables in intro.go

First pass of the clarification renaming:

1. Changed all type structs were Pascal Case
2. Expanded 2-3 character field names for structs
3. Tried removing redundant functions

Add comments and update function names

Add new comments and function names for better code clarity

Updated go go project files

Undid struct naming that pushed stuff to be public

Undid the public struct scoping
@rkrishnasanka
Copy link
Contributor Author

@dosco I know this took a long time but I basically undid all the public structs and helped rename a lot of the variables to get a better idea about the flow.

If this is good, I'll restart the multischema support, we definitely need it on our end

@dosco dosco marked this pull request as ready for review July 13, 2024 01:44
@dosco dosco merged commit a8403d2 into dosco:master Jul 13, 2024
5 of 6 checks passed
@dosco
Copy link
Owner

dosco commented Jul 13, 2024

Thats a huge PR did you manually add these comments or is there a tool that helps?

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.

3 participants