patterncsharpModerateCanonical
Multithreaded Mandelbrot Generator Ver 2
Viewed 0 times
vermandelbrotmultithreadedgenerator
Problem
Update: Version 3 is here.
My first version was an answer I provided to EBrown for his original post titled "Multithreaded Mandelbrot Generator". My answer had many good things in it, but I felt some of parts of the code were weak, particular around my efforts to apply some flexible scaling rather than using a constant of 2. This version addresses those weaknesses, tightens up some code, and simplified the multithreading even more.
```
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Drawing;
using System.Drawing.Imaging;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
// Original CodeReview Question:
// https://codereview.stackexchange.com/questions/104171/multithreaded-mandelbrot-generator
// Interesting link about coloring:
// http://www.fractalforums.com/programming/newbie-how-to-map-colors-in-the-mandelbrot-set/
// Trusty Wikipedia:
// https://en.wikipedia.org/wiki/Mandelbrot_set
namespace Mandelbrot_Generator
{
public class MandelbrotGeneratorV2
{
// Readonly properties to be set in constructor
public int Width { get; }
public int Height { get; }
public short MaxIterations { get; }
public float ScaleFactor { get; }
private short[] _iterationsPerPixel = null;
private Point _center;
private SizeF _scaleSize;
private float _scaleSquared;
public MandelbrotGeneratorV2(int height, short maxIterations, float scaleFactor = 2.0F)
{
// Use some very basic level limit checking using some arbitrary (but practical) limits.
const int heightLow = 512;
const int heightHigh = 4096 * 2;
const short iterationLow = 100;
const short iterationHigh = 32000;
const float scaleLow = 1.0F;
const float scaleHigh = 8.0F;
CheckLimits(nameof(height), height, heightLow, heightHigh);
CheckLimits(nameof(maxIterations), maxIte
My first version was an answer I provided to EBrown for his original post titled "Multithreaded Mandelbrot Generator". My answer had many good things in it, but I felt some of parts of the code were weak, particular around my efforts to apply some flexible scaling rather than using a constant of 2. This version addresses those weaknesses, tightens up some code, and simplified the multithreading even more.
```
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Drawing;
using System.Drawing.Imaging;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
// Original CodeReview Question:
// https://codereview.stackexchange.com/questions/104171/multithreaded-mandelbrot-generator
// Interesting link about coloring:
// http://www.fractalforums.com/programming/newbie-how-to-map-colors-in-the-mandelbrot-set/
// Trusty Wikipedia:
// https://en.wikipedia.org/wiki/Mandelbrot_set
namespace Mandelbrot_Generator
{
public class MandelbrotGeneratorV2
{
// Readonly properties to be set in constructor
public int Width { get; }
public int Height { get; }
public short MaxIterations { get; }
public float ScaleFactor { get; }
private short[] _iterationsPerPixel = null;
private Point _center;
private SizeF _scaleSize;
private float _scaleSquared;
public MandelbrotGeneratorV2(int height, short maxIterations, float scaleFactor = 2.0F)
{
// Use some very basic level limit checking using some arbitrary (but practical) limits.
const int heightLow = 512;
const int heightHigh = 4096 * 2;
const short iterationLow = 100;
const short iterationHigh = 32000;
const float scaleLow = 1.0F;
const float scaleHigh = 8.0F;
CheckLimits(nameof(height), height, heightLow, heightHigh);
CheckLimits(nameof(maxIterations), maxIte
Solution
I think that your limit checking is a little obscure.
Why are the constants defined in the constructor? They're related to your class, not to your constructor. If I were to be able to change the
You might want to look at the Contract class in the .Net Framework. It offers functionalities that would be useful to you. Otherwise, if you don't want to use this, you should at least rename the method
Comments like this :
Are useless in code. They should be in a product backlog or something like that (An issue on your Github, a note on a napkin on the side of your desk). Keep as little comments as necessary. But I understand, right now, that it means this code might not need to be reviewed as it will change.
Don't use brackets when it's not necessary like here :
First, it's confusing. Maybe you forgot to write a condition, maybe a for? I don't know, and it's confusing me. Remove those brackets.
Find that reason, lol. That seems weird. And the second part of your comment is useless, I don't mind if it's true even if bla bla bla. I know why you used
Overall, your code is good! Some of your comments are excellent as they explain why you do something instead of what you're doing. But the other comments should be removed.
Why are the constants defined in the constructor? They're related to your class, not to your constructor. If I were to be able to change the
Width or Height someday, I'd need these constants to validate that my new values are still "legit".You might want to look at the Contract class in the .Net Framework. It offers functionalities that would be useful to you. Otherwise, if you don't want to use this, you should at least rename the method
CheckLimits to something like AssertValueInRange or something close to this. Right now, CheckLimits isn't very self-explanatory.Comments like this :
// Coloring is probably has the greatest potential for further improvements.
// This is just one attempt that suffices for now.Are useless in code. They should be in a product backlog or something like that (An issue on your Github, a note on a napkin on the side of your desk). Keep as little comments as necessary. But I understand, right now, that it means this code might not need to be reviewed as it will change.
Don't use brackets when it's not necessary like here :
{
var lastIndex = sections.Length - 1;
var startY = heightPerSection * lastIndex ;
sections[lastIndex] = new Section(new Point(0, startY), new Point(Width, Height));
}First, it's confusing. Maybe you forgot to write a condition, maybe a for? I don't know, and it's confusing me. Remove those brackets.
// The sectionWidth is the same value as Width but for some odd reason
// using Width is noticeably faster on my 8-core PC. This is true even
// if I create a local copy such as:
// var sectionWidth = section.Width;Find that reason, lol. That seems weird. And the second part of your comment is useless, I don't mind if it's true even if bla bla bla. I know why you used
Width, that was a good comment, but no need to explain too much. Remember, minimal comments.Overall, your code is good! Some of your comments are excellent as they explain why you do something instead of what you're doing. But the other comments should be removed.
Code Snippets
// Coloring is probably has the greatest potential for further improvements.
// This is just one attempt that suffices for now.{
var lastIndex = sections.Length - 1;
var startY = heightPerSection * lastIndex ;
sections[lastIndex] = new Section(new Point(0, startY), new Point(Width, Height));
}// The sectionWidth is the same value as Width but for some odd reason
// using Width is noticeably faster on my 8-core PC. This is true even
// if I create a local copy such as:
// var sectionWidth = section.Width;Context
StackExchange Code Review Q#113606, answer score: 11
Revisions (0)
No revisions yet.