Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Oligopoly#72

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
ZacharyPatten merged 18 commits intodotnet:mainfromFuinny:main
May 11, 2023
Merged

Oligopoly#72

ZacharyPatten merged 18 commits intodotnet:mainfromFuinny:main
May 11, 2023

Conversation

@Fuinny
Copy link
Contributor

Hello! I made my own game called Oligopoly. This is a CLI simulator of the stock market of companies. I've only added the game files so far. I will be glad to receive any feedback! Thank you for your time!

@ZacharyPatten
Copy link
Collaborator

ZacharyPatten commentedMay 10, 2023
edited
Loading

Howdy! Thanks for the interest in contributing. :) I'll review this pull request as soon as I can.

I can add in the code to make the game playable in the website and the readme updates, so don't worry about adding those.

#17

Fuinny reacted with thumbs up emoji

@ZacharyPattenZacharyPatten changed the titleOligopoly #17OligopolyMay 10, 2023
@ZacharyPatten
Copy link
Collaborator

ZacharyPatten commentedMay 11, 2023
edited
Loading

I made some changes to the code:

  • I removed the link to your github profile from the console output because github usernames can be changed. If you ever changed your username the link would be broken. However, your username will appear on the readme when you hover over the "community contribution" link and you will be listed as a contributor once this PR is merged in. :)
  • There was a bug when calling theDisplayMoreAboutCompaniesMenu method. The code would jump to the next event if you clicked that option rather than just jumping back to the previous menu. That is fixed now.
  • I removed the hard coding of the Black and White console colors, because the user may not be using Black and White colors on their console. Now the code just inverts the current foreground and background colors which will work with any color settings the user has.
  • I added abool Program.CloseRequested field, and any time the user presses the Escape key, the program will exit. This gives the user a way to close the game at any time. Also it is better to avoid usingEnvironment.Exit in favor of just letting the code end.
  • I switched from usingData.xml toCompany.json andEvent.json becuase json tends to be more popular nowadays than xml and it made the code a lot shorter. There is nothing wrong with xml, but I just thought this was better.
  • Instead of having the data file copied to the output directory on build, it is now an embedded resource. This is better as the files are embedded in the executable rather than being standalone files; without it being an embedded resource, the app would fail if the file was not in the proper location. There are pros & cons to using embedded resorces but it is better in this case as I will have to embed it in the Website code regardless.
  • I moved all the code to the root directory of the project (rather than being inSource andData subfolders). This is primarily to keep it consistent with the other games in the repository.
  • I wrapped the letters from the board of directors in a box to try to make it look more like a letter.
  • I moved the table above the current event during the output of the main game loop, because people are going to press enter to rapidly progress through the game, and having the table below the current event means it will jump up and down because the current events are not all the same length. Therefore if we put the table at the top of the view, it's position will be consistent and make it easier to follow each change visually.
  • I swapped theThread.Sleep calls forConsole.ReadKey calls. It is just better and faster to let the player progress the game at there own pace.
  • I swapped out all the recursive method calling for simple loops. In general you want to prefer loops over using recursion because you can eventually get stack overflow exceptions while using recursion.
  • I removed and/or renamed some of the properties on theEvent andCompany classes and updated the xml/json data files in attempts to simplify it. Now it just uses the name of the company instead of "Ticker".

I feel like a "Weight" of 3 on the main readme would probably be appropriate for this game, but feel free to sugguest a different weight. The weight rating is pretty arbitrary; just something to encourage new developers to start at the top of the list.

Hopefully the reason why I made each change is clear, but if you have any question or disagree with any of my changes feel free to ask or give me feedback. :)

I still need to make some additional changes before the code is ready to pull in.

@ZacharyPattenZacharyPatten added the community contributionGames originally contributed by members of the community. Thank you! labelMay 11, 2023
@Fuinny
Copy link
ContributorAuthor

Fuinny commentedMay 11, 2023
edited
Loading

Thank you for improving my code and thanks for helping me make my first contribution! I think that I will try to implement some of your changes myself in my repository.

About the link to my profile, yes this is my mistake, I did not notice that I did not delete it. Thanks for fixing it!

Yes, I think 3 is the appropriate value for "Weight" for this project.

Thanks again!

Frames around letters look cool btw :)

@ZacharyPatten
Copy link
Collaborator

ZacharyPatten commentedMay 11, 2023
edited
Loading

I made some additional changes. Sorry I didn't follow great coding practices and ended up bundling them into one large commit, but here are some comments:

  • ChangedCompany.SharePrice &money fromdouble todecimal becausedecimal is the prefered data type for financial data.decimal does not round data within it's range of values butdouble, orfloat, will.
  • Changed the win/lose conditions to be based off the player's current "net worth" rather than their current money. This just seemed more logical. If you dump all your money into a company that tanks, then the board of directors would likely fire you. They wouldn't wait for you to sell the stocks before doing so.
  • Applied currency formatting to the output.
  • I changed how the buy & sell menus work. Instead of only using the up and down arrows I added the ability to buy multiple shares from multiple companies at the same time with the right and left arrows. I also added limits so that the user cannot buy more shares than they can afford or sell more shares than they have. I felt this was a better user experience.
  • I added the website version of the game. :)
  • I removed the "Positive" and "Negative" fields on events as they are not needed. You can just set the "Effect" to be positive or negative.
  • RemovedGameMenu andMenu in favor of static methods in theProgram.cs file. While using polymorphism worked, I think it was overkill for something as simple as a console menu. Also, this made it easier to port the code into the website.
  • I made the market events a separate screen/confirmation from the user menu. I felt like this keeps the user interface cleaner/simpler and thus probably easier for the user to understand.

As before, feel free to provide feedback if you disagree with any of the changes I made, but I felt like each one was an improvement.

@ZacharyPatten
Copy link
Collaborator

ZacharyPatten commentedMay 11, 2023
edited
Loading

I believe this pull request is ready to merge in. :) The links in the readme files will work once the code is merged in.

@Fuinny, since I made quite a few changes, do you have any comments/changes you want to make before I merge in this pull request?

Keep in mind if there are any bug fixes or changes that need be made in the future you can always open a new pull request with the updates.

@Fuinny
Copy link
ContributorAuthor

I think that your changes will make the game more interesting for the players. Yes, you can merge this pull request and I hope that this game will be an interesting addition to the repository.

Thank you for your time and interest in my game! 👍

ZacharyPatten reacted with thumbs up emoji

@ZacharyPattenZacharyPatten merged commit2ce55de intodotnet:mainMay 11, 2023
@ZacharyPatten
Copy link
Collaborator

ZacharyPatten commentedMay 11, 2023
edited
Loading

Thanks for the awesome contribution and congrats on your first merged pull request! 🎉

Your game is now playable in the website (for which the current link ishttps://dotnet.github.io/dotnet-console-games/Oligopoly). :)

It can take a while for GitHub to update the list of contributors on the repository, but you should be listed as a contributor once it updates.

Fuinny reacted with hooray emoji

Fuinny pushed a commit to Fuinny/dotnet-console-games that referenced this pull requestJun 26, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@ZacharyPattenZacharyPattenZacharyPatten approved these changes

Assignees

No one assigned

Labels

community contributionGames originally contributed by members of the community. Thank you!

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Fuinny@ZacharyPatten

[8]ページ先頭

©2009-2025 Movatter.jp