Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite
1

I've got this code that I found in a project and I'm wondering how can it be done better:

- (void) setAlpha:(float)alpha {
    if (self.superview.tag == noDisableVerticalScrollTag) {
        if (alpha == 0 && self.autoresizingMask == UIViewAutoresizingFlexibleLeftMargin) {
            if (self.frame.size.width < 10 && self.frame.size.height > self.frame.size.width) {
                UIScrollView *sc = (UIScrollView*)self.superview;
                if (sc.frame.size.height < sc.contentSize.height) {
                    return;
                }
            }
        }
    }
    if (self.superview.tag == noDisableHorizontalScrollTag) {
        if (alpha == 0 && self.autoresizingMask == UIViewAutoresizingFlexibleTopMargin) {
            if (self.frame.size.height < 10 && self.frame.size.height < self.frame.size.width) {
                UIScrollView *sc = (UIScrollView*)self.superview;
                if (sc.frame.size.width < sc.contentSize.width) {
                    return;
                }
            }
        }
    }

    [super setAlpha:alpha];
}
share|improve this question
1  
Woah there. That's a big set of conditions. It would be more helpful to know context: what do these do, why are they here, and what do they apply to? It may be possible to break this up into different components by using more OOP functionality, but without that, it's hard to help. –  Jake King Mar 29 at 18:10

2 Answers

up vote 4 down vote

First of all, using the tag to distinguish between views is almost everytime (there are exceptions) a bad idea. It's much cleaner to use your own variable (pointers to view, BOOL flags).

10 is a magic number. What does it mean? Use a constant.

Use methods. Four levels of if are terrible. Extract the code into methods.

if ([self isVerticalScrollingDisabled] || [self isHorizontalScrollingDisabled]) {
    return;
}

[super setAlpha:alpha];

By the way, changing the behavior of a method in a subclass (the method sets alpha only if some conditions are valid) is smelly from architecture point of view. Declaring a special method, e.g. setAlphaIfScrollingEnabled: would be better.

share|improve this answer
1  
Exactly, it violates LSP –  Denis Mikhaylov Apr 28 at 10:20
 
+1. For example, they could use KVO to listen for alpha changes, and perform the side effects then, instead of overriding setAlpha:. –  Nate May 24 at 20:56
up vote 0 down vote

That’s almost unreadable. Or, more precisely, it’s plain what the code does, but it’s almost impossible to guess the purpose. Generally, there are two conditions that prevent the view from taking a new alpha setting, right? Some general remarks:

  • The noDisableHorizontalScrollTag constant deserves a better name. It’s a good idea to mark constants with uppercase initial letter (think UIViewAutoresizingFlexibleHeight, CGRectZero and similar), but more importantly the double negation (no disable) is needlessly hard to parse. Why not EnableHorizontalScrollingTag? Or something similar, better fitting your use case.

  • If possible, it’s good to avoid changing view behaviour according to its superview. Especially when you assume here that the superview is always a UIScrollView. Maybe you could introduce some “delegate” or “data source” style pointer to the view to read the constraints from? That would also express the type constraint in the type system, making it possible for the compiler to check the code for you. Again, hard to say more without understanding your use case.

But above all, you should introduce some higher-level concept to make the code readable. Like wrap the frame-checking logic into a named method, where the name would be some meaningful concept (isVerticalScrollingEnabled, for example). Otherwise it’s very hard to find the connection between alpha, autoresizing mask, superview’s frame and its content size.

But maybe we’re just missing context here.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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