patterncsharpMinor
Generating Heighway dragons in Unity C#
Viewed 0 times
dragonsgeneratingunityheighway
Problem
I'm basically looking for a general critique. I'm just starting out and would like to know if I should have done anything differently to make it faster or to make it much more readable. I don't know exactly what happened but I tried it with 14 iterations and it took about 3 minutes to complete. It crashed Unity at 15 and after a restart, completed 15 iterations in half a second. I have a feeling I should just cut the dragon list and add the vertices to the Line Renderer as they're generated but I feel like the way it is now might be easier to read.
Are there any more corners I could be cutting? I lost indentation on the body of the class when I pasted it into the codeblock.
```
using UnityEngine;
using System.Collections;
using System.Collections.Generic;
public class HeighwayGenerator : MonoBehaviour {
private List direction;
private List dragon;
private Stack stack;
private LineRenderer line;
private float magnitude = 0.0f;
public int iterations = 4;
void Start () {
direction = new List ();
stack = new Stack ();
dragon = new List ();
line = GetComponent ();
//generate turn list
generateDirection (iterations);
//set start direction. Vector3.forward is axis of rotation (0,0,1)
transform.Rotate (Vector3.forward, (float)iterations * 45.0f);
//find segment length of fully iterated list. magnitude of 0th iteration is 10 units from (-5,0) to (5,0)
magnitude = 10.0f / (Mathf.Pow (Mathf.Sqrt (2), iterations));
//generate vertices and fill dragon
generateVertices ();
//draw line
line.SetVertexCount (dragon.Count);
for (int i = 0; i < dragon.Count; i++) {
line.SetPosition(i, dragon[i]);
}
}
void generateDirection(int _iteration) {
// Heighway dragon is generated from an initial direction and a list of turns.
// This list is generated by:
// 1.Pushing previous list into stack.
// 2.Appending 'R'
// 3.Appending reversed and 'negated?' previous list from stack.
if
Are there any more corners I could be cutting? I lost indentation on the body of the class when I pasted it into the codeblock.
```
using UnityEngine;
using System.Collections;
using System.Collections.Generic;
public class HeighwayGenerator : MonoBehaviour {
private List direction;
private List dragon;
private Stack stack;
private LineRenderer line;
private float magnitude = 0.0f;
public int iterations = 4;
void Start () {
direction = new List ();
stack = new Stack ();
dragon = new List ();
line = GetComponent ();
//generate turn list
generateDirection (iterations);
//set start direction. Vector3.forward is axis of rotation (0,0,1)
transform.Rotate (Vector3.forward, (float)iterations * 45.0f);
//find segment length of fully iterated list. magnitude of 0th iteration is 10 units from (-5,0) to (5,0)
magnitude = 10.0f / (Mathf.Pow (Mathf.Sqrt (2), iterations));
//generate vertices and fill dragon
generateVertices ();
//draw line
line.SetVertexCount (dragon.Count);
for (int i = 0; i < dragon.Count; i++) {
line.SetPosition(i, dragon[i]);
}
}
void generateDirection(int _iteration) {
// Heighway dragon is generated from an initial direction and a list of turns.
// This list is generated by:
// 1.Pushing previous list into stack.
// 2.Appending 'R'
// 3.Appending reversed and 'negated?' previous list from stack.
if
Solution
Some quick coding style remarks:
-
I'm not a fan of K&R style brackets, and it's a style rarely seen in C# code.
-
I like to see access modifiers on everything. Makes things a lot clearer.
-
-
You use the opposite of the usual C# style of having underscores in front of private instance field names, instead you use them for parameters. This is very confusing.
-
-
Why do you leave a space between your method name and the bracket, e.g.
-
-
Comment should tell me why, not what. If the method call is
-
But this code can be even simpler:
The same remark as the previous one can be said of this code:
You seem to have copy-pasted three lines which are almost identical for both cases, except for one parameter. If you're doing that, you should extract those lines into a method, or if you feel that's overkill you should simplify your code:
-
I'm not a fan of K&R style brackets, and it's a style rarely seen in C# code.
-
I like to see access modifiers on everything. Makes things a lot clearer.
-
generateDirection, generateVertices etc: method names should be in PascalCase.-
You use the opposite of the usual C# style of having underscores in front of private instance field names, instead you use them for parameters. This is very confusing.
-
'L' and 'R' are used repeatedly, so why not convert these into constants?-
Why do you leave a space between your method name and the bracket, e.g.
direction.Add ('R');?-
direction and dragon are Lists, yet their names do not indicate that they are collections. Collection names should be plural, IMHO. I'd even argue this should be the case for Stack stack, though that is a borderline case.-
Comment should tell me why, not what. If the method call is
generateVertices();, I shouldn't need to be informed //generate vertices and fill dragon -- unless your method name doesn't tell me everything I need to know.-
generateDirection starts with a multi-line comment explaining what the method does. I'd expect such an explanation in the `, not in the body of the method.
-
public int iterations = 4; Why is this public? Why isn't this a const? Is it your intention that this amount can be changed by code calling the HeighwayGenerator? If so, than it should be a property with a getter/setter, or perhaps you should keep iterations private and only allow it to be changed through a method.
Concerning this logic:
for (int i = 0; i < _tempCount; i++) {
char _tempChar = stack.Pop ();
if (_tempChar == 'R') {
direction.Add ('L');
}
if (_tempChar == 'L') {
direction.Add ('R');
}
}
I fail to see how _tempChar (a badly named variable, by the way) can be anything other than 'L' or 'R', so the second if can just as well be an else`.But this code can be even simpler:
for (int i = 0; i < _tempCount; i++) {
direction.Add((stack.Pop() == 'R') ? 'L' : 'R');
}The same remark as the previous one can be said of this code:
if(direction[i] == 'R'){
transform.Rotate (Vector3.forward, -90.0f);
transform.position += magnitude * transform.right;
dragon.Add (transform.position);
}
if (direction[i] == 'L') {
transform.Rotate (Vector3.forward, 90.0f);
transform.position += magnitude * transform.right;
dragon.Add (transform.position);
}You seem to have copy-pasted three lines which are almost identical for both cases, except for one parameter. If you're doing that, you should extract those lines into a method, or if you feel that's overkill you should simplify your code:
for (int i = 0; i < direction.Count; i++) {
transform.Rotate (Vector3.forward, (direction[i] == 'R' ? -90.0f : 90.0f));
transform.position += magnitude * transform.right;
dragon.Add (transform.position);
}Code Snippets
for (int i = 0; i < _tempCount; i++) {
char _tempChar = stack.Pop ();
if (_tempChar == 'R') {
direction.Add ('L');
}
if (_tempChar == 'L') {
direction.Add ('R');
}
}for (int i = 0; i < _tempCount; i++) {
direction.Add((stack.Pop() == 'R') ? 'L' : 'R');
}if(direction[i] == 'R'){
transform.Rotate (Vector3.forward, -90.0f);
transform.position += magnitude * transform.right;
dragon.Add (transform.position);
}
if (direction[i] == 'L') {
transform.Rotate (Vector3.forward, 90.0f);
transform.position += magnitude * transform.right;
dragon.Add (transform.position);
}for (int i = 0; i < direction.Count; i++) {
transform.Rotate (Vector3.forward, (direction[i] == 'R' ? -90.0f : 90.0f));
transform.position += magnitude * transform.right;
dragon.Add (transform.position);
}Context
StackExchange Code Review Q#94244, answer score: 5
Revisions (0)
No revisions yet.