Keeping my new code clean

Discussion in 'Mac Programming' started by larswik, Jun 24, 2011.

  1. larswik macrumors 68000

    Sep 8, 2006
    Tonight I really got into the Cocoa learning and it did not seem that hard. I got my random d100 dice roller working (I play RoleMaster with friends :)) where it keeps track of the last 4 roles. I also added a slider which I could set from values 1 to 5 to pick the highest roll out of X numbers of rolls.

    I created 1 class and all the code from my IBOutlet and IBActions went in to. I wrote no code from main except what it came with when the program launched.

    Here is what I am wondering:

    Should I be writing more of this code in main.m and sending messages to different object methods, or is writing everything in 1 class normal? Pascal class was teaching me to send things out of main into Functions to process and keep my main part clean and easier to read.

    Here is my class implementation
    #import "theDice.h"
    @implementation theDice
    int lastDiceRolled = 0;
    int secondDiceRolled = 0;
    int thirdDiceRolled = 0;
    int forthDiceRolled = 0;
    int sliderValue[5];
    int bestNumber, numberOnSlider,theRandomRollValue;
        [slider setIntValue:1];
        [numRollsToDo setIntValue:[slider intValue]];
    - (IBAction)theButton:(id)sender {
        bestNumber = 0;
        for (int i=0; i <= 4; i++) {
            sliderValue[i]= 0;
        numberOnSlider = [slider intValue];
        for (int i=0; i < numberOnSlider; i++) {
            theRandomRollValue = arc4random() %100 +1;
            sliderValue[i] = theRandomRollValue;
            if (bestNumber <= theRandomRollValue) {
                bestNumber = theRandomRollValue;
        NSString* str = [NSString stringWithFormat:@"%d", bestNumber];
        [lable  setStringValue:str];
        NSString* last = [NSString stringWithFormat:@"%d", lastDiceRolled];
        [lastRoll  setStringValue:last];
        NSString* second = [NSString stringWithFormat:@"%d", secondDiceRolled];
        [secondRoll  setStringValue:second];
        NSString* third = [NSString stringWithFormat:@"%d", thirdDiceRolled];
        [thirdRoll  setStringValue:third];
        NSString* forth = [NSString stringWithFormat:@"%d", forthDiceRolled];
        [forthRoll  setStringValue:forth];
        forthDiceRolled = thirdDiceRolled;
        thirdDiceRolled = secondDiceRolled;
        secondDiceRolled = lastDiceRolled;
        lastDiceRolled = bestNumber;
        NSString *sliderTextSpaceOne =[NSString stringWithFormat:@"%d", sliderValue[0]];
        [rOne setStringValue:sliderTextSpaceOne];
        NSString *sliderTextSpaceTwo =[NSString stringWithFormat:@"%d", sliderValue[1]];
        [rTwo setStringValue:sliderTextSpaceTwo];
        NSString *sliderTextSpaceThree =[NSString stringWithFormat:@"%d", sliderValue[2]];
        [rThree setStringValue:sliderTextSpaceThree];
        NSString *sliderTextSpacefour =[NSString stringWithFormat:@"%d", sliderValue[3]];
        [rFour setStringValue:sliderTextSpacefour];
        NSString *sliderTextSpacefive =[NSString stringWithFormat:@"%d", sliderValue[4]];
        [rFive setStringValue:sliderTextSpacefive];
    - (IBAction)bestOutOfRolls:(id)sender {
        [numRollsToDo setIntValue:[slider intValue]];
        numberOnSlider = [slider intValue];
    It seems a bit cluttered and I was just wondering, how to clean it. Jim showed me a nice wargame code clean up. I am just wondering what I could do to make this look nicer, even though it functions.

    Thanks for any guidance.


    Attached Files:

    • roll.jpg
      File size:
      30.7 KB
  2. jiminaus macrumors 65816


    Dec 16, 2010
    Start by not using global variables. Global variables are bad. One with only one exception you should never use global variables.

    Move all those variables after @implementation into the @interface so that they are instance variables instead.
    @interface theDice : NSObject
       // TODO: Move global variables to here
    Change the name of the class. theDice is bad for to reasons (at least). It doesn't start with a capital letter. "the" seems to imply (at least to me) an object, not a class. What you've actually got here is a controller in the Model-View-Controller architecture. So perhaps call it Controller (or MyController).

    Think about what the theButton: does, not who triggers it. See if you can think of a name that describes what it does. Methods normally start with a verb.

    theButton: clearly does a sequence of separate things. Chop the code up and move it out into individual methods that do one things and one things only. Then call those individual methods in sequence inside theButton:.

    Oh, and in a typical Cocoa application, main is nothing more that what is already provided by XCode.
  3. balamw Moderator


    Staff Member

    Aug 16, 2005
    New England
    A quick comment on .... comments!

    Particularly when you code looks cluttered you need to comment it. If nothing else this helps establish boundaries for you to refactor into new methods/functions/procedures.

  4. wpotere Guest

    Oct 7, 2010
    Yes please, use comments!

    I know that you know what the code is doing but it is a good habit to start. I took over a project at the office from a person that used no comments and had no documentation. There are a lot of sections that I have no full understanding of what they are doing and why.

    As for main, if you have a function (I'm not mac programmer) in main that can be put in a method of its own, it is best to do it.
  5. NickFalk macrumors 6502


    Jun 9, 2004
    A word of caution though (at the risk of sounding like a contrarian): Don't over-comment. Chosing good names for your ivars, methods and classes should make a lot of the stuff obvious without the need of comments.

    If there's too many comments people have a tendency of scanning past them instead of reading them.
  6. wpotere Guest

    Oct 7, 2010
    I agree but I would rather a developer write a novel rather than leave nothing.
  7. gnasher729 macrumors P6


    Nov 25, 2005
    "Fourth" and "label".

    Should the anti-spelling correction police be reading this: We are discussing code here, and we are discussing programming, Larswik asked for advice to keep his code clean, correct spelling is part of clean code, so bugger off.

    Did you intend to make the variables at the top of the file available to the whole world? If not, make them "static". As a rule: Either you declare it in a header file with the "extern" keyword, or you make it static.
  8. NickFalk macrumors 6502


    Jun 9, 2004
    A “trick” I find improves the readability of my code is to use tab on both sides of a = when setting variables. Makes it really easy to spot where you are changing those values.
  9. PatrickCocoa macrumors 6502a

    Dec 2, 2008
    Random Numbers

    Some comments on clean code:
    1. Set up your method to receive the maximum die roll as an input. In your case that would be 100.

    2. Consider setting up your method to receive these as input: 1) minimum die roll; 2) number of dice; 3) number of past rolls to save. These are not as necessary as #1, and will complicate your program, but may be helpful.

    Some comments on working with random numbers:
    1. Remember that you're using pseudorandom numbers, not random numbers;

    2. The topic of random numbers is huge - you don't need to read any of the thousands of books on the topic for this program, but be aware that there are some non-obvious things going on;

    3. Set up your code to have the ability to start the pseudorandom seed with a value you specify in code. That way you'll generate the same sequence of numbers each run, which is absolutely required for a given debugging session. Change this seed every when you start a new debugging session in order to hit different parts of your code.

    4. Set up some way to store and analyze a large set of your random output (maybe a million or so). In your case, set up some separate code to save every die roll, then analyze that data for reasonableness. The two most common problems this will uncover are: 1) clipping the highest or lowest random number (in your case 1 or 100); 2) using some non-uniform distribution (in your case that would result in more die rolls in the 40 to 60 range than in the outer ranges).
  10. dejo Moderator


    Staff Member

    Sep 2, 2004
    The Centennial State
    More comments on comments:

    Rather than documenting the 'how' of something (usually that should be obvious from the actual code), make an effort to document the 'why' (that's usually less obvious to other readers of the code, or yourself, weeks later).
  11. notjustjay macrumors 603


    Sep 19, 2003
    Canada, eh?
    Definitely. Don't make comments like "Initialize the value of X" and "Copy X to Y". We can see that from reading the code itself. These kinds of comments tell us nothing.

    Better comments might be "Set up the default inventory" or "Keep the running total in Y for averaging later" -- they help you to understand why you're doing it.

    As a bonus, later when you're trying to maintain large code files you can do searches on keywords like "inventory" or "total" and it will help you find things faster.
  12. chown33, Jun 24, 2011
    Last edited: Jun 24, 2011

    chown33 macrumors 604

    Aug 9, 2009
    Sailing beyond the sunset
    Coding Guidelines for Cocoa covers naming and other things.

    Cocoa Fundamentals Guide covers Cocoa design patterns. You can use them in your own code, too. Things like delegates can be quite useful.

    Object-Oriented Programming with Objective-C discusses classes, objects, and the relationships between them. A good design is about the relationships as much as it's about the classes.


    Your code looks cluttered because it repeats the same wordy pattern many times with slight variations. It's called copy-pasta. This repetition should be a big clue to factor it out as a function or a private method. Private methods defined in a category were already pointed out to you in another thread. A private function would work just as well here.

    This is one sample of repetitive code:
        NSString* third = [NSString stringWithFormat:@"%d", thirdDiceRolled];
        [thirdRoll  setStringValue:third];
    Factored out into a function:
    static NSString *
    decimalString( int n )
    {  return [NSString stringWithFormat:@"%d", n];  }
    and the function in use:
        [thirdRoll  setStringValue:decimalString( thirdDiceRolled )];
    Also, think about whether to use the setIntValue: method, thus eliminating the need for a function or private method, yet condensing all that clutter and extra local variables into one-liners like this:
        [rOne setIntValue:sliderValue[0]];
        [rTwo setIntValue:sliderValue[1]];
    The methods are defined in NSControl.

    Finally, when posting code, always post the @interface for the class. Otherwise we have to guess which variables are instance variables, and what their types are.
  13. larswik thread starter macrumors 68000

    Sep 8, 2006
    Wow, I was not expecting that much feedback. The original version was HelloWorld cocoa excessive. That spiraled out of control with me saying"It would be cool if I added this". At the end it looked like a code mess and I did no pre-planning and didn't know if I was even writing things in the right area.

    Jim - "Start by not using global variables" Opps, that was an over site.
    - I will use better naming conventions. I got in the bad habit of writing camel hump for everything.

    balamw - "Comments" Yes, I know about comments. Last night I seemed to get in to this strange rhythm of writing code (in the zone so to speak), that I just plain forgot to comment what I was doing. While not overly commenting like NickFalk mentioned.

    gnasher729 - haha. I am Dyslexic and write fast, 2 things that are not good. But, I guess it is good that I can constantly write it the wrong way multiple times :) Also I have not seen "extern" mentioned in the books yet that I have been reading from. I'll back track and read up on it.

    PatrickCocoa - "Set up your method to receive the maximum die roll as an input" huh? I set it to %100. I know this is the % modulas (remainder) but I don't know what is really happening to get the numbers?

    - "pseudorandom numbers, not random numbers" huh again? I don't understand what the difference is between them. Pseudorandom seems to imply sometimes random? can you point me to documentation to read up? I would like to learn more on this subject.

    chown33 - "Your code looks cluttered because it repeats the same wordy pattern many times" YES, that is what I noticed to. that's when I though there must be away to send this out to process and return the result, but where I wondered? another method with in this object. I was unsure if a method could send to another method within the same object. Thank you very much for including that extra code to do this. Also thanks for your pointer variables to the documentation objects you always list :) I learn so much from the interaction with the great folks on this board that I can't always get from documentation alone.


  14. balamw Moderator


    Staff Member

    Aug 16, 2005
    New England
    :p I know you do.

    Here's my general rule of thumb. Well named, compact (< ~20 line of code) blocks of code require 0-1 lines of comments. Anything else requires ~1 comment every 20 lines or so. Functional blocks that are potentially separable deserve a comment.

    This generally takes you to a place where if you delete all the code and leave in only the comments and function/method declarations the "what" and "why" of the code is still understandable, even though you have removed the "how". (as dejo also said earlier).

    It's just an opportunity for refactoring. :p

  15. larswik thread starter macrumors 68000

    Sep 8, 2006
    balamw - Yes. I am happy with the final result of my little program. I want to now focus on cleaning and moving it around to make it more readable. I suppose I could make a class and call it the 'function' class where I could send things to process. It feels like my AppController which I called 'theDice' became my new main.m, that felt weird. I was so use to Pascal and C to send things out of main, and now I wrote everything from within a class. Jim said that was kind of normal from his response.

  16. balamw Moderator


    Staff Member

    Aug 16, 2005
    New England
    You'll find that it's generally easier to maintain code that is kept clean as you develop it rather than doing clean-up after the fact. ;)

  17. mydogisbox macrumors member

    Jan 16, 2011
    Its good to ask questions like the one that started this thread, but for questions like this searching a bit yourself is better. One is an analysis question which is difficult to answer without experience, and the other is a definition question which you can simply look up.
  18. larswik thread starter macrumors 68000

    Sep 8, 2006
    Those habits are good to start learning now then later. When you design code for projects do you have typical patterns that you use? Expl. Knowing when to send to functions?

  19. balamw Moderator


    Staff Member

    Aug 16, 2005
    New England
    Not always.

    Often times I will essentially write my pseudo code as a bunch of comments and then flesh this out. If a particular section becomes too lengthy or repetitive, consider a new function or method.

    Larger projects will involve a lot of time doing planning and writing a specification before any code is ever written.

  20. chown33 macrumors 604

    Aug 9, 2009
    Sailing beyond the sunset
    My first impression is that doing so would be a sign of poor factoring.

    Things should be in the same class because they have a logical relationship to the instances of that class, or to the class itself. Unless there's a close logical relationship between the things being processed, or the actions that perform the processing, then there's no reason to put things into the same class.

    For example, suppose there's a Dice class. Two things always related to dice: number of sides, and a method to perform a roll of the dice. So it has an immutable parameter to -initWithSides, and a -roll method that returns a randomly chosen number consistent with the number of sides. A deck of cards is another type of gaming device with random outputs. However, you would not put a dealRandomCard or an array of cards in a Dice class, because those are unrelated to the object being simulated. If there's a valid reason to have a RandomGamingDevice class, which is the superclass of the Dice and CardDeck classes, then that would be the connection that relates the two. But there should be a reason to have a RandomGamingDevice in first place; generating randomness isn't sufficient reason.

    You'll also want to read the difference between is-a and has-a.


    Car is-a Vehicle. Motorcycle is-a Vehicle. Car has-a 4 Wheel instances. Motorcycle has-a 2 Wheel instances.

    Elf is-a RPGCharacter. Elf has-a String for its name. Elf has-a collection of Weapon instances: 0 or more, perhaps constrained by character's strength and sum of Weapon weights.
  21. PatrickCocoa, Jun 24, 2011
    Last edited by a moderator: Jun 24, 2011

    PatrickCocoa macrumors 6502a

    Dec 2, 2008
    Wikipedia is where I would have started too, but having just read "Pseudorandom number generator", that article is not completely suited for someone without a mathematical background. The one on "Random number generation" would be more helpful for the original poster. Also check out:

    Quick summary: pseudo random numbers are generated by a formula, usually something like:
    Pseudo Random Number (t) = (Constant #1 * Pseudo Random Number(t-1) )+ Constant #2] mod Constant #3)
    That's what the arc4random function is doing - from your perspective it's returning random numbers, but what it's really doing is running through the formula above.

    I'd like to reiterate my comments on:
    1. Debugging (set the seed so you can get the same set of numbers each session);
    2. Test your output!
    3. Check that you actually do get die rolls of 1, and 100, and don't get any die rolls of 0 or 101.

    Yes, the arc4random takes the 100 as an input, but what I meant was to set up a method, then you send that method the maximum die roll. For example:

    -(int) randomDieRoll:(int) maximumDieRoll
        // insert comments here to keep balamw happy.
        // actually you'll need the comments to keep future Lars happy
        return arc4random() %maximumDieRoll +1;
  22. larswik thread starter macrumors 68000

    Sep 8, 2006
    Thanks Guys.

    Chown33- Thanks for showing me some short cuts and explaining dice/card classes and where things should go. I might spend my Friday night and expand that program a little by adding in an Experiences Point Calculator. Which sould be it's own class then. So main calls an NSwindow. I create a AppControler Class. Then I create sub classes for Dice and XP Calculator.

    balamw - My Pascal teacher could not stress enough about top-down designs. I could never get that concept of approaching a project that way. Pseudo code like you are talking about is what I seemed to do for those projects. Kind of outline what needs to happen. Thanks for the advice.

    PatrickCocoa - I wrote a little program for the arc4Random() using a 100 sided dice. It rolled the d100 1000 times and then it incremented a 100place index int array. Then with another for loop within a for loop I had it print a '*' for each time it rolled it so if number 35 was rolled 5 times I got "*****" starts. The result looked pretty random and I printed the result bellow. A visual representation was easier to see then a bunch of numbers.
  23. larswik thread starter macrumors 68000

    Sep 8, 2006
    I thought I could just take the app that was made in when I pressed BUILD and send it to a friend to open and check out. It did not work. Researching on line almost everything I could find was iPhone and for Xcode3 and all the steps to make a release version, holy cow.

    If anyone has any links for Xcode4 and a non ios (Mac OX 10.4 or better) please let me know.

  24. jiminaus, Jun 24, 2011
    Last edited: Jun 24, 2011

    jiminaus macrumors 65816


    Dec 16, 2010
    In XCode 4:
    1. Build | Archive
    2. Window | Organizer
    3. Archives tab
    4. Select the project
    5. Share...
    6. Application
    7. Choose place to store your app

    Remember that a Mac OS X app is actually a folder, so you can't sent it directly. You have to compress it into either a zip file or a disk image.

    To make a zip:
    1. Open Finder
    2. Find where you stored your app
    3. Right-click/control+click the app
    4. Compress

    To make a .dmg disk image:
    1. Open Disk Utility
    2. File | New | Disk Image From Folder...
    3. Select the app
    4. Choose where to save the .dmg file
  25. larswik thread starter macrumors 68000

    Sep 8, 2006
    Thanks Jim, I will try it in the morning, it's late here


Share This Page