smoothbrain420 / OOP-code

0 stars 0 forks source link

Code review #1

Open MuchMarts opened 1 year ago

MuchMarts commented 1 year ago

I will try my best to do a code review for easier reviewing I suggest creating a pull request thus allowing code reviewers directly comment the code thus allowing you to get a better understating of what can and should be improved

Documentation

There is plenty documentation. I would say that method "PrintPackOrder()" does not have to be commented out and can be left as an extra method. Only places where more documentations could be added is in shuffling algorithms you could add a short description of what the algorithm is doing so in the future you have a better understating of the algorithm when reading the code again.

Error handling

Error handling can be improved upon. On line 16 you should check whether type of shuffle is one of the 3 and if not return an error message so the user knows that something is wrong and you should implement a genera shuffle method (as per brief shuffleCardPack) inside pack class that takes typeOfShuffle as a parameter and check whether its a correct input and then shuffles the pack. This would make the code more readable. As well as if implemented you should also check whether the shuffling was successful this can be achieved by having the general shuffle method return a boolean value if its was successful or not.

In method dealCard(int numCards) [line 152] you should check if there are enough cards left in the pack before dealing them to make it more clear that an error has occurred. Because if you do not then the user might try to deal more cards and it will work but not entirely this could result in further errors if not handled correctly when integrated with "clients" card games.

[line 144] Throwing and exception without a try catch will stop the execution of code you should either add a try catch to handle the exception OR instead return an error value and deal with the error where you actually use the method or you could write the console and error message. There are many ways to handle this error that do not stop the execution of the program.

Suggestions

When creating a OOP program it is good practice to have each class in separate files or grouped in separate files so its easier to read the code and develop it. As well as when developing a c# program AFAIK you need .Net framework with it similar to how the given program files had multiple additional files they help the program be executable. Currently AFAIK I could be wrong you can not run the program unless you create a new console application and then copy paste the code there.

Also you should have the Main() method that the code executes inside a Program class instead of running it from the Testing class this allows for code clarity and modularity. Because if in the future you would more features and classes/methods and you wanted to test a specific thing whilst your main program is running you could create a Testing object and call specific test methods from it. Rather than just running the whole program from Testing.

Other than that if you improve upon the code It should be good to go :D

smoothbrain420 commented 1 year ago

Documentation: I will be adding comments on how those algorithms work. Thank you for showing me that issue Error handling: I'll try my best to implement those error catches into my code Suggestions: I agree I probably should have put them in different programs but I was getting a lot of errors due to multiple entrances into the program on Visual studio. Out of desperation I threw it all onto LINQPAD to see if it could work properly. I'll try making seperate files but I honestly think its something to do with my Visual Studio Still thanks a lot for the thorough feedback. Its greatly appreaciated.

ProNewb commented 1 year ago

Your code has nice readability and lots of header comment with direct and clear explanation. You cold possibly include some inline comments to expand your documentation and explain some of the more compex algorithms such as the shuffles.

There is error handling in the code at multiple points covering the range possible exceptions and code that resolves the issue in a favourable way. The problem I had is the repo is saved as a single .linq file. There is no sln file and after cloning into vs has 14 errors and wont compile. Due to this fact I had to copy and paste into a new console project seperating each class just to run your code as a c# project and Iam not sure you would be able to submit the project in this form.

After testing functionality was almost all there and working correctly and the error handling was spot on.

The only base code function missing was the bool return function ShuffleCardPack. You have added compareable functionality so wether that costs points I do not know. My favourite part of your code was the throw new IndexOutOfRangeException for an empty deck. This is something i did not consider so ++.

The only real additions I could proffer would be point scoring exercises, for example you could add some inline coding. You could also add a test class and move your test code to it and call it from main() which is the normal entry point to a c# program. If you look on the crg and check list adding additional classes increases marks. It would also mean you have left the base code class Program in your code aswell.

I did notice you don't write out the complete deck just the hand, you could write out the test deck to show the deck has been created correctly.

Overall an excellent implementation I hope you score well.

smoothbrain420 commented 1 year ago

I see your point with moving the code into different programs and calling it all from the main method. honestly just struggled getting it to work in VS. until i figure out whats wrong ill just have to do it this way because the code just doesnt run. Thank you so much for your feedback I hope you do well.

ProNewb commented 1 year ago

I literally opened a new c# project added the releveant classes then cut and copied your code and it ran no issue so there isnt much to do to get it running mate

ProNewb commented 1 year ago

if you need further help just let me know mate ill do what i can. If you re-download the base code in the assessment 1 directory on blackboard and copy paste your code into the correct places. You already have it seperated into Card, Pack, Main its just in one block so do that and it will at least run like it did for me. Hopefully you will will get it sorted. good luck