snippetMinor
Simple plotting abstract base class and example subclass
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
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.
Your header exposes 19 properties and 4 methods. I didn't check all of them, but starting with
But you should carry around properties like this which aren't used, so get rid of them.
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
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
I'm not convinced everything in your
Don't abbreviate things. Especially things you're publicly exposing in the header file. What is
In the end, I think that this class could benefit greatly by taking a look at
```
@protocol ZFChartDataSource
@end
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.
- 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.
- 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.- Consistency is King.
strongis the default memory-management attribute for object properties. So how come on some of your properties you explicitly marked them asstrong, while others you omitted any memory-management attribute?
retainis the same asstrongthese days, and it does the same thing... but let's just usestrong, for consistency.
readwriteis the default accessibility attribute for properties. Why do some of your properties explicitly statereadwritewhile the rest omit this attribute and acceptreadwriteas default? The only time you should specifyreadwriteis when you want a property exposed in the header asreadonlyto be overridden asreadwritewithin the.mfile, or when you're simply marking every property with areadonly/readwriteattribute (for the consistency of explicitly marking all attributes).
nonatomicis not the default attribute. It's also the less-safe (betweenatomicandnonatomic). But there doesn't seem to be a good explanation for you having marked some of your properties asnonatomic. 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 usingCGFloatconsistently here, although in the case oftimeBetweenPoints, we're going to wantNSTimeIntervalas our type.
- Pick an asterisk style and stick to it. I personally prefer
ClassName instanceVariableName;, but you've used a combination of that andClassName instanceVariableName;
- 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.- 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;
@endContext
StackExchange Code Review Q#110293, answer score: 3
Revisions (0)
No revisions yet.