Skip to content

Conversation

@0spooky2me
Copy link
Collaborator

Created this to make showing current games and displaying server-side games easier. Just wanted to see what you thought about it.

Created game-sheet directory in UI folder
Sheet can be used for other purposes
};

/**
* Template predicate to determine if game is Japanese style
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@amrocha Do you think I'm overdoing the function documentation?

Copy link

Choose a reason for hiding this comment

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

Hmm yeah I think for like, self-evident function like this one it would be fine to not have anything, but you have type information there which really helps in vanilla JS so I think it adds something

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have no idea how useful type information is. I commented some of the functions in detail, and some I left uncommented. Which is better?

Copy link

Choose a reason for hiding this comment

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

Is this in preparation for other people to take over maintenance of the system? For example, in lines 60-181 it's hard to keep track of the code and see the big idea with the comments in between. If it's for people to sift through and figure out though, this much documentation may be necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I'm not going to be around forever so I think it might be important. Do you think it takes too much away?

Copy link

Choose a reason for hiding this comment

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

Give me a day or two, I'm gonna download it and try looking at it without as much documentation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With some editor setups you should be able to collapse comments

@0spooky2me
Copy link
Collaborator Author

@TwelveNights @Mooters @amrocha What do you think of the level of documentation here? Is it too much, or just enough for someone who is unfamiliar with the code?

Copy link
Collaborator

@chllarry chllarry left a comment

Choose a reason for hiding this comment

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

I think it clarifies the code lot more, and the formatting makes it easier to understand though not exactly easier to read. Still a good change overall for people unfamiliar with code or the way the menu works

@0spooky2me 0spooky2me requested a review from chllarry March 24, 2019 23:49
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.

6 participants