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

Added my group data structure #2

Closed
wants to merge 1 commit into from
Closed

Added my group data structure #2

wants to merge 1 commit into from

Conversation

AlexWilkinsonnn
Copy link

Si9hence
Si9hence reviewed Oct 22, 2020
View reviewed changes
Copy link

@Si9hence Si9hence left a comment

Choose a reason for hiding this comment

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

general data structure looks fine but with some problem in the connection part

the connection part needs to specify the relationship between two people but not shown in this case

ugne112
ugne112 reviewed Oct 22, 2020
View reviewed changes
Copy link

@ugne112 ugne112 left a comment

Choose a reason for hiding this comment

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

The connections are not specified, only names given

Si9hence
Si9hence reviewed Oct 22, 2020
View reviewed changes
Copy link

@Si9hence Si9hence left a comment

Choose a reason for hiding this comment

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

In case the group gets larger, there might be a conflict when two persons have the same name.

AhmadUCL
AhmadUCL reviewed Oct 22, 2020
View reviewed changes
group.py

"Nash" : {"age" : 34, "job" : "chef", "connection" : ["Zalika", "John"]}}


Copy link

Choose a reason for hiding this comment

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

[Suggestions] Perhaps add a list for the job key value so it can be empty (to represent people without a job). Also, it's good that you added a list for the connection because it can be empty, but you could use a dictionary within this list to give the the type of relation for each connection (to make the connections reciprocal). Well done, though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Si9hence Si9hence Si9hence left review comments

@AhmadUCL AhmadUCL AhmadUCL left review comments

@ugne112 ugne112 ugne112 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AlexWilkinsonnn @Si9hence @AhmadUCL @ugne112