HiveBrain v1.2.0
Get Started
← Back to all entries
snippetMinor

Simple plotting abstract base class and example subclass

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
simpleabstractplottingexamplesubclassandclassbase

Problem

I came across a CocoaControl ZFPlot and wanted to extend here. I ended up writing a base class (ZFPlot shown below) as well as several extensions (ZFLine, ZFScatter, ZFBar, none shown here for brevity). I'd really love feedback on everything in the code below but especially on readability, performance, and structure. I know it's a lot of code, so I welcome feedback even on small bits.

ZFPlotChart.h

```
#import
#import
#import "ZFDrawingUtility.h"
#import "ZFData.h"
#import "ZFConstants.h"

@interface ZFPlotChart : UIView

// Overall plot properties
@property CGFloat xAxisLabelType; // are x-axis labels are 0 = data array indices, 1 = NSDates, 2 = actual numerical values
@property BOOL convertX; // when true, x values are scaled rather than equally spaced, set TRUE for scatter plot only
@property BOOL gridLinesOn; // draw gridlines?
@property (nonatomic, strong) UIColor *baseColorProperty;
@property CGFloat xUnitWidth;

// Data to display and units to apply to the plotted y-axis values
@property CGFloat stringOffsetHorizontal; // move x-axis strings to the left to recenter
@property (nonatomic, retain) NSString *units; // y value units
@property (nonatomic, retain) NSString *xUnits; // x value units, only used if xAxisLabelType == 2

// Data controller
@property ZFData* dictDispPoint; // an ordered set of key-value pairs with fields corresponding to constants fzValue and fzXValue

// Drawing controller
@property ZFDrawingUtility *draw;

// Animation
@property float timeBetweenPoints;
@property BOOL animatePlotDraw;
@property int countDown;
@property NSMutableArray *alreadyIncluded;

// Layout properties for plotting the view
@property (nonatomic, readwrite) float chartWidth, chartHeight;
@property (nonatomic, readwrite) float leftMargin;

// Tracking all points in data as they are iterated over
@property (nonatomic, readwrite) CGPoint prevPoint, curPoint, currentLoc;
@property BOOL isMovement;

// Show when data is loading or missing
@property (strong) UIActi

Solution

There's a lot to comment on in the header alone... so I will...

We can worry about the innards later. What's most important when writing a class like this is that the public-facing aspect make as most sense as possible to users who are unfamiliar with exactly how to use your code.

  1. Removed anything unused.



Your header exposes 19 properties and 4 methods. I didn't check all of them, but starting with xAxisLabelType, there are some properties that aren't even used in your code. I wasn't explicitly checking for this. I was checking because the variables seemed poorly named, and the hope was that looking in the code would help me better discern their exact purposes.

But you should carry around properties like this which aren't used, so get rid of them.

  1. Empty methods are empty methods--not abstract methods.



If you want to give subclasses a chance do special things, you don't need to do anything special. All I have to do in my subclass is throw a few extra calls into my override of drawRect: or whatever else I want to modify.

Importantly, in Objective-C, there is no concept of "protected", which some other languages like Java have. A protected method can be overridden by, and seen by, a child class, but it can not be seen by the outside world. There's not a good way of accomplishing this task in Objective-C.

By putting drawSpecial in the header, all you've done is publicly expose a method that probably didn't need to be public at all. If it ever makes sense to mark a method as something that shouldn't be called by external classes (which would make sense here), then the right thing to do is simply not put it as a publicly available method.

  1. Consistency is King.



  • strong is the default memory-management attribute for object properties. So how come on some of your properties you explicitly marked them as strong, while others you omitted any memory-management attribute?



  • retain is the same as strong these days, and it does the same thing... but let's just use strong, for consistency.



  • readwrite is the default accessibility attribute for properties. Why do some of your properties explicitly state readwrite while the rest omit this attribute and accept readwrite as default? The only time you should specify readwrite is when you want a property exposed in the header as readonly to be overridden as readwrite within the .m file, or when you're simply marking every property with a readonly/readwrite attribute (for the consistency of explicitly marking all attributes).



  • nonatomic is not the default attribute. It's also the less-safe (between atomic and nonatomic). But there doesn't seem to be a good explanation for you having marked some of your properties as nonatomic. Why have you done this? Nothing in your code appears to have required you to do this. You did not write custom setters/getters.



  • float/CGFloat: There doesn't seem any logic dictating which you're going to use. I'd opt for using CGFloat consistently here, although in the case of timeBetweenPoints, we're going to want NSTimeInterval as our type.



  • Pick an asterisk style and stick to it. I personally prefer ClassName instanceVariableName;, but you've used a combination of that and ClassName instanceVariableName;



  1. Don't expose publicly things which are private.



I'm not convinced everything in your .h file even needs to be in your .h file. We can move some of it into a separate private interface in the .m file, affording us the closest thing to what Objective-C would call private properties.

  1. Good naming goes a long way.



Don't abbreviate things. Especially things you're publicly exposing in the header file. What is dictDispPoint? No one has any clue. This isn't even remotely a good descriptive name for a variable. And keep in mind that argument names are just that, argument names... and they don't count as part of the method name. So, createChartWith: is a bad method name, while createChartWithData: is better. It still needs work though, because that method's argument should be an NSData *, but it's actually an NSOrderedSet. So maybe we should call it createChartWithDataSet:?

In the end, I think that this class could benefit greatly by taking a look at UITableView and use a protocol/delegate approach. There's no indication what sort of objects should be in the ordered set I pass in the createChartWith: method. But if we had a protocol that exposed methods like:

```
@protocol ZFChartDataSource

  • (ZFChartStyle)styleForChartView:(ZFChartView *)chartView;
  • (NSInteger)numberOfDataSetsInChartView:(ZFChartView *)chartView;
  • (NSInteger)chartView:(ZFChartView *)chartView numberOfPointsInDataSetAtIndex:(NSInteger)dataSetIndex;
  • (UIColor )chartView:(ZFChartView )chartView colorForDataSetAtIndex:(NSInteger)dataSetIndex;
  • (ZFChartDataPoint )chartView:(ZFChartView )chartView dataPointForDataPath:(ZFDataPath)dataPath;



@end

Code Snippets

@protocol ZFChartDataSource

- (ZFChartStyle)styleForChartView:(ZFChartView *)chartView;
- (NSInteger)numberOfDataSetsInChartView:(ZFChartView *)chartView;
- (NSInteger)chartView:(ZFChartView *)chartView numberOfPointsInDataSetAtIndex:(NSInteger)dataSetIndex;
- (UIColor *)chartView:(ZFChartView *)chartView colorForDataSetAtIndex:(NSInteger)dataSetIndex;
- (ZFChartDataPoint *)chartView:(ZFChartView *)chartView dataPointForDataPath:(ZFDataPath)dataPath;

@end
@protocol ZFChartDelegate

- (void)chartView:(ZFChartView *)chartView didSelectDataPointAtDataPath:(ZFDataPath)dataPath;

@end

Context

StackExchange Code Review Q#110293, answer score: 3

Revisions (0)

No revisions yet.