5
\$\begingroup\$

I developed a Kuku Kube game for iOS. Each time I choose a correct different color tile, it will increase a number of tile quantity 2x2 3x3...10x10. How to optimise the code each time the tile increase that auto fit on ip4 to ip6 Plus? I think it can not use auto layout to set for the size.

- (void)setSizeforTile {
    tiles = (NSInteger)pow(level, 2);
    randomTile = arc4random() % tiles;
    sizeForCell = 0.0;
    switch (level) {
        case 2:
            if (IS_IPHONE_4_OR_LESS) [self setCollectionViewCellDetail:125 withItemSpacing:5];
            if (IS_IPHONE_5) [self setCollectionViewCellDetail:150 withItemSpacing:10];
            if (IS_IPHONE_6) [self setCollectionViewCellDetail:180 withItemSpacing:5];
            if (IS_IPHONE_6P) [self setCollectionViewCellDetail:200 withItemSpacing:4];
            break;
        case 3:
            if (IS_IPHONE_4_OR_LESS) [self setCollectionViewCellDetail:82 withItemSpacing:4.5];
            if (IS_IPHONE_5) [self setCollectionViewCellDetail:100 withItemSpacing:5];
            if (IS_IPHONE_6) [self setCollectionViewCellDetail:120 withItemSpacing:2.5];
            if (IS_IPHONE_6P) [self setCollectionViewCellDetail:133 withItemSpacing:2.5];
            break;
        case 4:
            if (IS_IPHONE_4_OR_LESS) [self setCollectionViewCellDetail:61.5 withItemSpacing:3];
            if (IS_IPHONE_5) [self setCollectionViewCellDetail:76 withItemSpacing:2];
            if (IS_IPHONE_6) [self setCollectionViewCellDetail:89 withItemSpacing:3];
            if (IS_IPHONE_6P) [self setCollectionViewCellDetail:99 withItemSpacing:2.5];
            break;
        case 5:
            if (IS_IPHONE_4_OR_LESS) [self setCollectionViewCellDetail:49 withItemSpacing:2.5];
            if (IS_IPHONE_5) [self setCollectionViewCellDetail:60 withItemSpacing:2];
            if (IS_IPHONE_6) [self setCollectionViewCellDetail:70 withItemSpacing:3];
            if (IS_IPHONE_6P) [self setCollectionViewCellDetail:78 withItemSpacing:3.5];
            break;
        case 6:
            if (IS_IPHONE_4_OR_LESS) [self setCollectionViewCellDetail:41 withItemSpacing:1.8];
            if (IS_IPHONE_5) [self setCollectionViewCellDetail:49 withItemSpacing:3];
            if (IS_IPHONE_6) [self setCollectionViewCellDetail:58 withItemSpacing:3.2];
            if (IS_IPHONE_6P) [self setCollectionViewCellDetail:65 withItemSpacing:2.8];
            break;
        case 7:
            if (IS_IPHONE_4_OR_LESS) [self setCollectionViewCellDetail:35.5 withItemSpacing:1];
            if (IS_IPHONE_5) [self setCollectionViewCellDetail:41.5 withItemSpacing:3];
            if (IS_IPHONE_6) [self setCollectionViewCellDetail:50 withItemSpacing:2.5];
            if (IS_IPHONE_6P) [self setCollectionViewCellDetail:56 withItemSpacing:2];
            break;
        case 8:
            if (IS_IPHONE_4_OR_LESS) [self setCollectionViewCellDetail:31 withItemSpacing:1];
            if (IS_IPHONE_5) [self setCollectionViewCellDetail:36 withItemSpacing:3];
            if (IS_IPHONE_6) [self setCollectionViewCellDetail:42.5 withItemSpacing:3];
            if (IS_IPHONE_6P) [self setCollectionViewCellDetail:48 withItemSpacing:2.2];
            break;
        case 9:
            if (IS_IPHONE_4_OR_LESS) [self setCollectionViewCellDetail:27.5 withItemSpacing:0.9];
            if (IS_IPHONE_5) [self setCollectionViewCellDetail:32 withItemSpacing:2.75];
            if (IS_IPHONE_6) [self setCollectionViewCellDetail:38 withItemSpacing:2.8];
            if (IS_IPHONE_6P) [self setCollectionViewCellDetail:43 withItemSpacing:2.1];
            break;
        case 10:
            if (IS_IPHONE_4_OR_LESS) [self setCollectionViewCellDetail:24.5 withItemSpacing:0.9];
            if (IS_IPHONE_5) [self setCollectionViewCellDetail:28.5 withItemSpacing:2.75];
            if (IS_IPHONE_6) [self setCollectionViewCellDetail:35 withItemSpacing:1.6];
            if (IS_IPHONE_6P) [self setCollectionViewCellDetail:39 withItemSpacing:1.5];
            break;
        default:
            break;
    }
    cellSize = CGSizeMake(sizeForCell, sizeForCell);
//    NSLog(@"%d-%.1f-%.1f",level, sizeForCell, cellLineSpacing);
}

- (void)setCollectionViewCellDetail:(CGFloat)itemCellSize withItemSpacing:(CGFloat)itemCellSpacing {
    sizeForCell = itemCellSize;
    cellLineSpacing=itemCellSpacing;
    itemSpacing = itemCellSpacing;
}

- (CGSize)collectionView:(UICollectionView *)collectionView layout:(UICollectionViewLayout*)collectionViewLayout sizeForItemAtIndexPath:(NSIndexPath *)indexPath {
    return cellSize;
}

- (void)randomColor {
    CGFloat hue = ( arc4random() % 256 / 256.0 );  //  0.0 to 1.0
    CGFloat saturation = ( arc4random() % 128 / 256.0 ) + 0.5;  //  0.5 to 1.0, away from white
    CGFloat brightness = ( arc4random() % 128 / 256.0 ) + 0.5;  //  0.5 to 1.0, away from black
    color = [UIColor colorWithHue:hue saturation:saturation brightness:brightness alpha:1];
    [self setSizeforTile];
}
\$\endgroup\$
3
  • 2
    \$\begingroup\$ we aren't going to trawl through your repo to review your entire project. Select classes from your project that you think could use a review and post the source of those here. \$\endgroup\$ Sep 9, 2015 at 15:38
  • \$\begingroup\$ This is my GameViewController class, each time I choose a correct different color tile, it will increase a number of tile quantity 2x2 3x3...10x10 \$\endgroup\$ Sep 9, 2015 at 16:06
  • \$\begingroup\$ I can't tell were basically any of your variables are declared, what their types are, or what's really going on. What's tiles, randomTile, or sizeForCell? How about cellSize? And then sizeForCell, cellLineSpacing, and itemSpacing? And cellSize? How about color? The only variables that are actually declared anywhere are your hue, saturation, and brightness. Are all of the variables I've mentioned instance variables? If so, why aren't you using properties (nevermind object-oriented techniques)? \$\endgroup\$
    – nhgrif
    Sep 10, 2015 at 11:59

1 Answer 1

6
\$\begingroup\$

So, I left a comment expressing confusion upon the source of your variables. Then I looked in the edit history and found this code in its original context on Github. That helped me to understand where your variables are coming from. It also helped me to understand your code overall, and let me just say, it is piled to the nines with problems that all stem from trying to write an Objective-C iOS app as if you were writing a C app for Windows 3.1. I will say that the absolute best advice I can offer is to spend some time learning object-oriented programming, spend some time learning Objective-C, and spend some time learning iOS design patterns, then scrap this and start from scratch.

But, with all of that said, let's address the code in this question specifically...


I want to start at the bottom, with this:

- (void)randomColor {
    CGFloat hue = ( arc4random() % 256 / 256.0 );  //  0.0 to 1.0
    CGFloat saturation = ( arc4random() % 128 / 256.0 ) + 0.5;  //  0.5 to 1.0, away from white
    CGFloat brightness = ( arc4random() % 128 / 256.0 ) + 0.5;  //  0.5 to 1.0, away from black
    color = [UIColor colorWithHue:hue saturation:saturation brightness:brightness alpha:1];
    [self setSizeforTile];
}

The bulk of this actually isn't too bad. If we take the first three and a half lines, we're doing okay-ish. I think the individual line comments are a little unnecessary, and the method as a whole would be better served with an overall comment explaining what sorts of shades to expect from this method. And that might also mean changing the method name, because from a method called randomColor, I would expect a truly random color which should include white, black, and all the grays in the middle.

The problems are:

  1. What we're doing with the color we're calculating
  2. Doing too many things (beyond just calculating the random color)
  3. Where we've put this code

The reason why all three of these things are a problem can be summed up with one simple question:

What happens if we want a random color elsewhere in our app?

The answer is that we have to either copy & paste this code elsewhere, or we have to then refactor it into what I'm about to suggest anyway, which is how we should be striving to write things in the first place.

This, instead of being a method in some random view controller, should instead be in a UIColor class category. Putting the code in the class category resolves all of the aforementioned problems. In a class category, it can't be doing too much because the category doesn't know about the things that are using it. They only know about it. So we can then only write the method in the way it makes sense to write it:

UIColor+Random.h

@interface UIColor (Random)

+ (UIColor *)randomNonGrayColor;

@end

UIColor+Random.m

#import "UIColor+Random.h"

@implementation UIColor (Random)

+ (UIColor *)randomNonGrayColor {
    CGFloat hue = (arc4random() % 256)/256.0;
    CGFloat saturation = ((arc4random() % 128)/256.0) + 0.5;
    CGFloat brightness = ((arc4random() % 128)/256.0) + 0.5;
    return [UIColor colorWithHue:hue
                      saturation:saturation
                      brightness:brightness
                           alpha:1];
}

@end

So now, whenever we need a random, non-gray color, we need to import this header into the file that needs it:

#import "UIColor+Random.h"

And then, where ever we need the color, we call this method:

UIColor *randomColor = [UIColor randomNonGrayColor];

The other thing I want to address is this comment:

I think it can not use auto layout to set for the size.

It may be true that you cannot use auto-layout here. But rather than making the assumption, you should try with all your might everything you possibly can to make it work with autolayout. And make everything you can work with autolayout. Only when you've proven to yourself that it is absolutely wholly impossible to use autolayout for something should you then turn around and attempt anything else.

How does this code look on the iPad Pro with its 12.9" screen @3x resolution? How will this code look in a year if the iPhone 7 has some different sort of screen size? How does your code work with the iPad's split screen and picture-in-picture modes? Every time we fail to use autolayout, we have written code that we are virtually guaranteeing will require readdressing that code again on a yearly basis. We should make sure we're doing all the right things when we're developing user interfaces for iOS.

We've done something far worse here though than just calculating some view based on what device we're on. We're calculating a subview of some view based on what device we're on. What if, in the future, we decide to resize the collection view? Now every single one of these size calculations plus all the ones we add in the future are all instantly wrong. Each time we change any tiny thing about any part of the UI on this screen, we have to go through and tweak dozens of lines of code to get the numbers just right.

How about some math?

Instead of a special case for each device, we should work on deriving a formula to calculate these sizes which is device independent. We know what size the collection view is, and we know how many cells we want to display on the collection view. These are the only two things that should matter when we're calculating the size of the cells. It shouldn't matter what device we're on.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.