I'm creating a report that will display some 60+ elements, each of them color-coded for quick analysis. I've ended up with a bunch of code like this:
Yes it works and gets the job done, but to me it looks awful. Way too much repetition. I can't help but feel that there has to be a cleaner, more concise way to do this, but this is where my lack of experience shows. So I'm wondering how the experts here would tackle this problem.
And just to complicate things, depending on the metric involved, the target might be to keep the measured value less than the goal, and for other metrics the goal may be a value that must be exceeded.
Suggestions on a better design, anyone? I've about 60 more metrics to write code for, so if I'm going to change lanes, now is the time to do it.
And in case you are wondering, all 60+ will not be in one class <shudder>. The metrics are grouped into 5 categories each with it's own servlet, bean, dao, and scoring class, so that helps organize it a bit.
"The good news about computers is that they do what you tell them to do. The bad news is that they do what you tell them to do." -- Ted Nelson
You need to generalize and parameterize. Seems like you need two methods: colorForTargetBelowGoal() and colorForTargetAboveGoal(). The parameters would be the goal, the current data item, and the previous data item (the specific data you want to compare, not the SafetyBean.
One thing I don't understand is why you would have the goal as a field for each data set. Mixing the goals numbers with the datapoint numbers in the same object doesn't seem like a good design choice. Aren't the goal numbers independent of the individual data set numbers? If they are independent, they should be in a separate object. In other words, do the goal values depend on the data being current or previous? I would think the goal numbers would be the same for current and previous. Seems to me that you should have three sets of data instead: the goals, the current measured, and the previous measured. With that, the code would be:
I don't know if I'd mix the colors with the data either. I'd probably choose to have colors tracked in a different object. In fact, since the logic for being above or below a goal seems fairly important, I'd probably rethink whether or not it's a good design to have the logic mainly around selecting a color for display. Seems like the color for display is only incidental and there are other more important things that need to use the same kind of logic.
Thank you, excellent ideas. That's the sort of bump in another direction that I needed. One reason I'm storing all the data in a single object is so at the end I have 5 beans that can put in the request for the jsp. Then my jsp ends up looking like this:
But since I don't display the goals on the page, they can be moved to a "goal" object as you suggest. While the goals may change from month to month, the scoring is always based on the current actual against the current goal, or the current actual against last months actual.
I see that you are also implying the use of the Decorator pattern. I'm not familiar with that one; I'll go read up on it right now and try to figure out how that works in this scenario.
Jk Robbins wrote:I see that you are also implying the use of the Decorator pattern.
Sorry, my bad. Didn't mean to imply Decorator Pattern at all and I'm not sure this is quite that either. Maybe "formatter" would have been a less misleading choice of name.
I don't think it helps to use value of "color" the way you have used it. The name you use should convey some meaning other than the actual color: maybe 'good' => 'green', 'fair' => 'yellow', 'poor' => red, 'neutral' => 'white'. This goes back to my thought that there is more meaning to the logic you had written before than just the color used to display the data. Maybe you should be using names like "ON_TRACK" (good/green), "SLIGHTLY_OFF" (fair/yellow), "OFF_TRACK" (poor/red), "NEUTRAL" instead to make the code read better.
One reason I'm storing all the data in a single object is so at the end I have 5 beans that can put in the request for the jsp.
You'll soon learn that it's better to keep different kinds of concerns separated. If you can isolate UI-related concerns like the colors used to render displays from other concerns then changes in one area tend to have less ripple effect into other areas of the code. Maintenance and refactoring is just much easier when you keep those things separate. I suppose it's something we each have to experience to see the value. Good luck.
Thanks for getting back with that, because I started reading on Decorator and went "Huh?".
I also already got away from the colors and I'm using "fail", "success", and "improved" ("open" or white being the default) which can be used as css classes and the color can be determined in the stylesheet.
I know you've made some changes since then, but looking at the original code, I can't figure out how the "white" branches would ever be reached. Seems like if the current measurement doesn't beat the goal, and it doesn't beat the previous measurement, then the only remaining possibility (which does not require any additional check at this point) is that it should be red.
So your SafetyBean has dozens of properties? This needs refactoring. Group properties like goal, color, and measured in one class, say, Item. Then your code will look like:
The next step is to create new method assignColor:
The last step is to keep Items in collections, rather than in fields, iterate that collections and apply assignColor in a loop.
One note on code logic: the third condition is negation of the first one, so if we checked the first and it yelds false, then the third will always be true. As a result, the 4th alternative will never happen, and white color will never be set.
Alexei Kaigorodov wrote:... create new method assignColor:
My problem with this is that it focuses too much attention on the color itself; the meaning of the comparisons gets lost or at least obfuscated. I think Jk is on the right track by thinking in terms of the other terms instead: fail, success, improved, and open -- the color itself is an incidental detail of the UI display concerns.
I also pondered the "white" branches for a while. I have a new class that looks like this.
I removed the test for the "open/white" option and I initialize it to "OPEN" just to give it a default value in case any of the other values aren't present.
The SafetyBean has 25 properties because a corresponding safety record in the database has 25 fields and I need all of them. The DAO fills one bean for each record. I'm not sure how refactoring is going to improve that scenario. I could have one bean for goals and one for measurements, but that doubles the number of database calls and I don't see how that's an improvement. It's already going to take 5 queries to build this page, I don't want to make it 10.
As for the earlier suggestion on Enum, I looked at a tutorial on that and I can't figure out any advantage to it. From what I read, Enums are most useful when used as parameters or when creating Singletons. But I have no experience with C\C++ which I believe is where Enums originated, so I'm sure that I'm not seeing the whole picture. What am I missing there?
I think that's a big improvement. I'd take the refactoring further. MetricScorer base class or interface with score() method that returns SUCCESS, FAILED, IMPROVED, or OPEN. Then two subclasses: first subclass scores SUCCESS when value is below the goal, the second subclass scores SUCCESS when value is above the goal. This way, you're calling the same score() method, and the way the score() method behaves is based on which subclass you're actually calling. This is more OO, IMO.
Junilu Lacar wrote:I think that's a big improvement. I'd take the refactoring further. MetricScorer base class or interface with score() method that returns SUCCESS, FAILED, IMPROVED, or OPEN. Then two subclasses: first subclass scores SUCCESS when value is below the goal, the second subclass scores SUCCESS when value is above the goal. This way, you're calling the same score() method, and the way the score() method behaves is based on which subclass you're actually calling. This is more OO, IMO.
Yeah, I like that idea. That's tomorrows project.
Joined: Mar 05, 2008
Jk Robbins wrote:I removed the test for the "open/white" option and I initialize it to "OPEN" just to give it a default value in case any of the other values aren't present.
Well, why would these other values not be present? Seems to me that will only happen if there's a coding error in the future. But you can also rely on the compiler to tell you if that happens, e.g.:
By declaring the local variable final, and not initializing it, we're telling the compiler it needs to be set exactly once in the subsequent if/else clauses. If someone screws that up in the future, they will get a compiler error pointing out the problem. If it compiles, we know the compiler has verified that part of our logic - the value will be initialized, and only once. Which is what we intend.