PDA

View Full Version : Keeping my new code clean




larswik
Jun 24, 2011, 03:43 AM
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;

-(void)awakeFromNib{
[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];
}

@end
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.

-Lars



jiminaus
Jun 24, 2011, 04:03 AM
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
}
@end


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.

balamw
Jun 24, 2011, 05:31 AM
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.

B

wpotere
Jun 24, 2011, 05:46 AM
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.

NickFalk
Jun 24, 2011, 06:14 AM
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.

wpotere
Jun 24, 2011, 07:01 AM
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.

I agree but I would rather a developer write a novel rather than leave nothing.

gnasher729
Jun 24, 2011, 07:29 AM
"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.

NickFalk
Jun 24, 2011, 07:43 AM
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.

PatrickCocoa
Jun 24, 2011, 09:07 AM
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).

dejo
Jun 24, 2011, 09:33 AM
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).

notjustjay
Jun 24, 2011, 10:45 AM
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).

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.

chown33
Jun 24, 2011, 10:55 AM
Coding Guidelines for Cocoa (http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/CodingGuidelines/CodingGuidelines.html) covers naming and other things.

Cocoa Fundamentals Guide (http://developer.apple.com/library/ios/#documentation/Cocoa/Conceptual/CocoaFundamentals/CocoaDesignPatterns/CocoaDesignPatterns.html) 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 (http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/OOP_ObjC/) discusses classes, objects, and the relationships between them. A good design is about the relationships as much as it's about the classes.


EDIT

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.

larswik
Jun 24, 2011, 04:08 PM
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.

Thanks!!!

-Lars

balamw
Jun 24, 2011, 04:26 PM
balamw - "Comments" Yes, I know about comments.


: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).


chown33 - "Your code looks cluttered because it repeats the same wordy pattern many times" YES, that is what I noticed to.


It's just an opportunity for refactoring. :p

B

larswik
Jun 24, 2011, 04:36 PM
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.

-Lars

balamw
Jun 24, 2011, 04:48 PM
I want to now focus on cleaning and moving it around to make it more readable.

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. ;)

B

mydogisbox
Jun 24, 2011, 05:22 PM
"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.

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.

http://en.wikipedia.org/wiki/Pseudorandom_number_generator

larswik
Jun 24, 2011, 05:23 PM
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. ;)

B

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?

-Lars

balamw
Jun 24, 2011, 05:35 PM
When you design code for projects do you have typical patterns that you use? Expl. Knowing when to send to functions?

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.

B

chown33
Jun 24, 2011, 05:40 PM
I suppose I could make a class and call it the 'function' class where I could send things to process.

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.
http://en.wikipedia.org/wiki/Is-a
http://en.wikipedia.org/wiki/Has-a

Examples:

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.

PatrickCocoa
Jun 24, 2011, 07:01 PM
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.

http://en.wikipedia.org/wiki/Pseudorandom_number_generator

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:
http://www.random.org/randomness/ (http://www.random.org/randomness)

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.

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?

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;

}

larswik
Jun 24, 2011, 09:34 PM
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.
*********1
*******2
***********3
**********4
**********5
**********6
***************7
*********8
*************9
***********10
******11
********************12
********13
*********14
************15
***********16
************17
*********18
******19
**********20
*********************21
*********22
*********23
******24
*********25
********26
************27
*************28
************29
*********30
***********31
**************32
***********33
************34
*****35
*************36
**********37
*********38
**************39
*************40
********41
***********42
*****43
*********44
*****45
******46
**********47
******************48
******************49
*********50
**********51
**********52
*********53
******54
**************55
******56
**********57
****************58
**********59
***********60
***********61
************62
******63
********64
*********65
*****66
****************67
*******68
*****************69
********70
***********71
************72
**********73
*************74
************75
**********76
******77
*********78
************79
****80
************81
*********82
******83
**********84
********85
**************86
***87
****88
***********89
********90
******91
****92
**********93
**********94
********95
**************96
********97
******98
**********99
************100

larswik
Jun 24, 2011, 10:37 PM
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.

-Lars

jiminaus
Jun 24, 2011, 10:51 PM
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.

-Lars

In XCode 4:

Build | Archive
Window | Organizer
Archives tab
Select the project
Share...
Application
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:

Open Finder
Find where you stored your app
Right-click/control+click the app
Compress


To make a .dmg disk image:

Open Disk Utility
File | New | Disk Image From Folder...
Select the app
Choose where to save the .dmg file

larswik
Jun 25, 2011, 03:02 AM
Thanks Jim, I will try it in the morning, it's late here

-Lars