3

I'm writing the simple card game "War" for homework and now that the game works, I'm trying to make it more modular and organized. Below is a section of Main() containing the bulk of the program. I should mention, the course is being taught in C#, but it is not a C# course. Rather, we're learning basic logic and OOP concepts so I may not be taking advantage of some C# features.

bool sameCard = true;

while (sameCard)
{
    sameCard = false;
    card1.setVal(random.Next(1,14));        // set card value
    val1 = determineFace(card1.getVal());   // assign 'face' cards accordingly
    suit = suitArr[random.Next(0,4)];       // choose suit string from array
    card1.setSuit(suit);                    // set card suit
    card2.setVal(random.Next(1,14));        // rinse, repeat for card2...
    val2 = determineFace(card2.getVal());    
    suit = suitArr[random.Next(0,4)];        
    card2.setSuit(suit);  

    // check if same card is drawn twice:

    catchDuplicate(ref card1, ref card2, ref sameCard); 
}
Console.WriteLine ("Player: {0} of {1}", val1, card1.getSuit());
Console.WriteLine ("Computer: {0} of {1}", val2, card2.getSuit());

// compare card values, display winner:

determineWinner(card1, card2);   

So here are my questions:

  • Can I use loops in Main() and still consider it modular?
  • Is the card-drawing process written well/contained properly?
  • Is it considered bad practice to print messages in a method (i.e.: determineWinner())?

I've only been programming for two semesters and I'd like to form good habits at this stage. Any input/advice would be much appreciated.

Edit:

catchDuplicate() is now a boolean method and the call looks like this:

sameCard = catchDuplicate(card1, card2);

thanks to @Douglas.

ChrisWue
  • 18,612
  • 4
  • 58
  • 83
nvillec
  • 61
  • 1
  • 8
  • Not sure if this is really a StackOverflow question. This is sort of a best-practices question, which can be subjective, but tends to have a consensus. – Sion Sheevok Apr 07 '12 at 20:16
  • 1
    Does `catchDuplicate` ever change the values of `card1` and `card2`? If not, then you should change its signature: `sameCard = catchDuplicate(card1, card2);` – Douglas Apr 07 '12 at 20:17
  • @SionSheevok oh alright, if this should be moved I'd be more than happy to. – nvillec Apr 07 '12 at 20:20
  • @Douglas you're right, it doesn't. Nice catch, thank you! – nvillec Apr 07 '12 at 20:21

4 Answers4

3

If the main problem of your homework is create a modular application, you must encapsulate all logic in specialized classes. Each class must do only one job. Function that play with the card must be in a card class. Function that draw cards, should be another class.

I think it is the goal of your homework, good luck!

sergiogarciadev
  • 2,061
  • 1
  • 21
  • 35
  • 1
    Normally, in "best practice"-OOP, each method does only one job. A class contains a related group of jobs that operate on the same data (contained in fields/properties). – Abel Apr 07 '12 at 20:26
  • 3
    +1. Yes, basically the `Main` method should only instantiate a game class and start the game. something like `var game = new CardsGame(); game.Run(); Console.ReadKey();`. – Olivier Jacot-Descombes Apr 07 '12 at 20:27
  • 1
    As the question is tagged **homework**, we must try to show him the path, not take his hand and walk the path. Nowadays I see a lot of developers who only now to follow recipes, not to program. I have hope in this padawan. – sergiogarciadev Apr 07 '12 at 20:30
3

Can I use loops in Main() and still consider it modular?

Yes, you can. However, more often than not, Main in OOP-programs contains only a handful of method-calls that initiate the core functionality, which is then stored in other classes.

Is the card-drawing process written well/contained properly?

Partially. If I understand your code correctly (you only show Main), you undertake some actions that, when done in the wrong order or with the wrong values, may not end up well. Think of it this way: if you sell your class library (not the whole product, but only your classes), what would be the clearest way to use your library for an uninitiated user?

I.e., consider a class Deck that contains a deck of cards. On creation it creates all cards and shuffles it. Give it a method Shuffle to shuffle the deck when the user of your class needs to shuffle and add methods like DrawCard for handling dealing cards.

Further: you have methods that are not contained within a class of their own yet have functionality that would be better of in a class. I.e., determineFace is better suited to be a method on class Card (assuming card2 is of type Card).

Is it considered bad practice to print messages in a method (i.e.: determineWinner())?

Yes and no. If you only want messages to be visible during testing, use Debug.WriteLine. In a production build, these will be no-ops. However, when you write messages in a production version, make sure that this is clear from the name of the method. I.e., WriteWinnerToConsole or something.

It's more common to not do this because: what format would you print the information? What text should come with it? How do you handle localization? However, when you write a program, obviously it must contain methods that write stuff to the screen (or form, or web page). These are usually contained in specific classes for that purpose. Here, that could be the class CardGameX for instance.

General thoughts
Think about the principle "one method/function should have only one task and one task only and it should not have side effects (like calculating square and printing, then printing is the side effect).".

The principle for classes is, very high-level: a class contains methods that logically belong together and operate on the same set of properties/fields. An example of the opposite: Shuffle should not be a method in class Card. However, it would belong logically in the class Deck.

Abel
  • 56,041
  • 24
  • 146
  • 247
  • 1
    I really like this answer. Thanks for the clear and detailed explanations! My level of understanding in OOP is limited so far (my class is just now moving out of the procedural realm), but this has helped immensely. – nvillec Apr 07 '12 at 21:13
2

Take all advices on "best practices" with a grain of salt. Always think for yourself.

That said:

  • Can I use loops in Main() and still consider it modular?

The two concepts are independent. If your Main() only does high-level logic (i.e. calls other methods) then it does not matter if it does so in a loop, after all the algorithm requires a loop. (you wouldn't add a loop unnecessarily, no?)

As a rule of thumb, if possible/practical, make your program self-documenting. Make it "readable" so, if a new person (or even you, a few months from now) looks at it they can understand it at any level.

  • Is the card-drawing process written well/contained properly?

No. First of all, a card should never be selected twice. For a more "modular" approach I would have something like this:

while ( Deck.NumCards >= 2 )
{
   Card card1 = Deck.GetACard();
   Card card2 = Deck.GetACard();
   PrintSomeStuffAboutACard( GetWinner( card1, card2 ) );
}
  • Is it considered bad practice to print messages in a method (ie: determineWinner())?

Is the purpose of determineWinner to print a message? If the answer is "No" then it is not a matter of "bad practice", you function is plain wrong.

That said, there is such a thing as a "debug" build and a "release" build. To aid you in debugging the application and figuring out what works and what doesn't it is a good idea to add logging messages.

Make sure they are relevant and that they are not executed in the "release" build.

Andrei
  • 4,880
  • 23
  • 30
  • Thanks for your answers! Your explanation of the method's purpose cleared things up. I _have_ added logging messages before to see where in the program something was happening, wasn't aware this was an actual practice haha. – nvillec Apr 07 '12 at 21:02
  • 1
    +1 for "making code readable". @nvillec: these and a lot more best practices can be found in the very readable and accessible book [The Pragmatic Programmer](http://www.amazon.com/The-Pragmatic-Programmer-Journeyman-Master/dp/020161622X). A landmark that I can highly recommend you (don't look at the publication date, _everything_ in there is still applicable). – Abel Apr 08 '12 at 10:20
1

Q: Can I use loops in Main() and still consider it modular?

A: Yes, you can use loops, that doesn't really have an impact on modularity.

Q: Is the card-drawing process written well/contained properly?

A: If you want to be more modular, turn DrawCard into a function/method. Maybe just write DrawCards instead of DrawCard, but then there's an optimization-versus-modularity question there.

Q: Is it considered bad practice to print messages in a method (ie: determineWinner())?

A: I wouldn't say printing messages in a method is bad practice, it just depends on context. Ideally, the game itself doesn't handle anything but game logic. The program can have some kind of game object and it can read state from the game object. This way, you could technically change the game from being text-based to being graphical. I mean, that's ideal for modularity, but it may not be practical given a deadline. You always have to decide when you have to sacrifice a best practice because there isn't enough time. Sadly, this is all too often a common occurrence.

Separate game logic from the presentation of it. With a simple game like this, it's an unnecessary dependency.

Sion Sheevok
  • 4,057
  • 2
  • 21
  • 37
  • +1 for detailed answers. Thanks for the in-depth explanation! I had a `drawCard` method for a while that contained the suit and value assignments, and called it for each card. However, ideally every method should only be responsible for one task right? Where is the line drawn, is "drawing a card" one task or would the individual `set` methods be the tasks. I guess what I don't quite understand yet is what defines a single task. – nvillec Apr 07 '12 at 20:45