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

Oligopoly #72

Merged
merged 18 commits into from May 11, 2023
Merged

Oligopoly #72

merged 18 commits into from May 11, 2023

Conversation

Fuinny
Copy link
Contributor

@Fuinny Fuinny commented May 10, 2023

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 commented May 10, 2023

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

@ZacharyPatten ZacharyPatten changed the title Oligopoly #17 Oligopoly May 10, 2023
@ZacharyPatten
Copy link
Collaborator

ZacharyPatten commented May 11, 2023

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 the DisplayMoreAboutCompaniesMenu 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 a bool 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 using Environment.Exit in favor of just letting the code end.
  • I switched from using Data.xml to Company.json and Event.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 in Source and Data 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 the Thread.Sleep calls for Console.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 the Event and Company 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.

@ZacharyPatten ZacharyPatten added the community contribution Games originally contributed by members of the community. Thank you! label May 11, 2023
@Fuinny
Copy link
Contributor Author

Fuinny commented May 11, 2023

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 commented May 11, 2023

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:

  • Changed Company.SharePrice & money from double to decimal because decimal is the prefered data type for financial data. decimal does not round data within it's range of values but double, or float, 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.
  • Removed GameMenu and Menu in favor of static methods in the Program.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 commented May 11, 2023

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
Contributor Author

Fuinny commented May 11, 2023

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 ZacharyPatten merged commit 2ce55de into dotnet:main May 11, 2023
2 checks passed
@ZacharyPatten
Copy link
Collaborator

ZacharyPatten commented May 11, 2023

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 is https://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 pushed a commit to Fuinny/dotnet-console-games that referenced this pull request Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community contribution Games originally contributed by members of the community. Thank you!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants