Add Huffman in C#. #82

june128 · 2018-04-19T00:10:52Z

ToDo

zsparal · 2018-04-21T21:44:23Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+{
+    public class EncodeResult 
+    {
+        public List<bool> BitString { get; set; }


This isn't much more efficient than a regular String and it's much more annoying to debug and print. If you seriously want to produce a packed binary result then I'd go with System.Collections.BitArray but I think a regular String works better for educational purposes.

I will go for the BitArray then. I don't think a string is fitting in this case, since no real compression would be achieved.

The point isn't compression, it's showcasing thr algorithm. And there was no compression achieved with List<bool> either. I'd just go with String (as almost all other implementations in the AAA do).

You mean since we usually save whole bytes anyway?

Strings in C# are UTF16 encoded which is 2 bytes. If you save the bitstring as a List<bool> that's N bytes, where N is the length of the bitstring. That means you literally only have any compression if the bitstring is a single bit long.

zsparal · 2018-04-21T21:46:46Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+        // The Node class used for the Huffman Tree.
+        public class Node
+        {
+            public Node[] Children { get; set; } = new Node[2];


Just have two children, Left and Right. This only makes it easier to misuse the class

zsparal · 2018-04-21T21:47:13Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+            public Node[] Children { get; set; } = new Node[2];
+            public List<bool> BitString { get; set; } = new List<bool>();
+            public int Weight { get; set; }
+            public string Key { get; set; }


char would work here

For the key?

Yes. Maybe char? so you can set it to null in the branches

The key of a node is string, so that a parent node's/branch's key is a combination of all it's children's keys.

zsparal · 2018-04-21T21:52:52Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+
+            public void AddNode(Node newNode)
+            {
+                if (Nodes.Count == 0)


Just have Node implement IComparable<Node> then use something like:

var insertAt = Math.Max(Nodes.BinarySearch(newNode), 0); Nodes.Insert(insertAt, newNode)

zsparal · 2018-04-21T21:55:47Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+            public List<Node> Nodes { get; private set; } = new List<Node>();
+
+            public NodePriorityList() { }
+            public NodePriorityList(List<Node> nodes) => Nodes = nodes.OrderByDescending(n => n.Weight).ToList();


Have Node implement IComparable and just do:

public NodePriorityList(List<Node> nodes) { Nodes = nodes.ToList(); Nodes.Sort(); }

zsparal · 2018-04-21T21:57:17Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+
+namespace HuffmanCoding
+{
+    public class EncodeResult 


Maybe a better name would be EncodingResult

zsparal · 2018-04-21T21:59:59Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+                    currentNode = currentNode.Children[1];
+
+                // Check if it's a leaf node.
+                if (currentNode.Key.Count() == 1)


Just check if all of the children are null. That's how you determine the leaves in a binary tree

zsparal · 2018-04-21T22:04:29Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+        {
+            // Create a List of all characters and their count in input by putting them into nodes.
+            var nodes = new List<Node>();
+            foreach (var character in input)


There's a nicer way to do this. Pseudo-ish code with a toy example:

var input = "Hello, World"; var nodes = input .GroupBy(c => c) .Select(n => new Node { Key = n.Key, Weight = n.Count() }) .ToList();

zsparal · 2018-04-21T22:06:54Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+            // Create Tree.
+            while (nodes.Count > 1)
+            {
+                var parentNode = new Node("", 0);


You should have a default constructor so you don't need to pass dummy parameters. I would actually have something like:

public class Node { public static Node CreateLeaf(char key, int weight) { return new Node(...); } public static Node CreateBranch(Node left, Node right) { return new Node(...); } private Node(...) { /* regular Node constructor with every parameter */ } }

I could also create two constructors for the Node class. One that takes key and weight and another one that takes a leftChild and rightChild. Your approach seems more declarative tho. What do you think?

zsparal · 2018-04-21T22:08:24Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+                    nodes.RemoveAt(nodes.Count - 1);
+                };
+                nodePriorityList.AddNode(parentNode);
+                if (parentNode.Weight > 100)


This shouldn't be here, the weights are not percentages

That's left from debugging and I forgot to remove it :S

zsparal · 2018-04-21T22:10:34Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+                // Add the two nodes with the smallest weight to the parent node and remove them from the tree.
+                for (int i = 0; i < 2; i++)
+                {
+                    parentNode.Children[i] = nodes.Last();


Make a nice method for this on the priority queue (and don't use the regular list at this point). Something like:

var left = nodeQueue.Pop(); var right = nodeQueue.Pop(); nodeQueue.AddNode(Node.CreateBranch(left, right)); // the weight will be calculated in `CreateBranch` as `left.Weight + right.Weight`

What are the improvements with using a queue? If I use it BinarySearch() wouldn't be available. I mean, I can re-implement that probably, but if the queue doesn't provide improvements, I see no reason to do so.

zsparal · 2018-04-21T22:11:12Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+            }
+            // Convert list of nodes to a NodePriorityList.
+            var nodePriorityList = new NodePriorityList(nodes);
+            nodes = nodePriorityList.Nodes;


Don't do this, just use the queue. In fact, just make Nodes private

So the Node class?

No, the Nodes property on NodePriorityList

zsparal · 2018-04-21T22:13:28Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+            return nodePriorityList.Nodes[0];
+        }
+
+        private static Dictionary<char, List<bool>> CreateDictionary(Node root)


I know I'm usually a big proponent for performance, but seriously, just use the recursive method here. With C# 7's local functions you can even make a nice, local function for the recursive part

Hmm, I'll take a look, but I found the non-recursive version easy to understand. Didn't tried the recursive one tho.

june128 · 2018-04-22T02:27:35Z

Thanks for the review @Gustorn ! I will enhance the code in the following days.

…bugging. Add mention.

…the readableBitString to console. Have Node implement IComparable<Node> and improve the code according to that. Improve the creation of the initial list of nodes. Refactor NodePriorityList. Improve Node creation.

zsparal · 2018-05-28T09:16:58Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+        {
+            var output = "";
+            Node currentNode = result.Tree;
+            foreach (var boolean in result.BitString)


You should probably name this bit

zsparal · 2018-05-28T09:18:46Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+            public int Weight { get; set; }
+            public string Key { get; set; }
+
+            // Creates a leaf. So just a node is created with the given values.


These comments are totally superfluous

Why? They are explaining what is done pretty well.

zsparal · 2018-05-28T09:19:40Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+{
+    public class EncodingResult 
+    {
+        public List<bool> BitString { get; set; }


I still think just having this be a string would make things a lot easier

zsparal · 2018-05-28T09:21:02Z

chapters/data_compression/huffman/code/cs/Program.cs

+            // Print dictionary.
+            foreach (var entry in result.Dictionary)
+            {
+                var bitString = "";


This is just: string.Join("", entry.Value.Select(bit => bit ? '1' : '0')), but if you take the advice of just using strings, this whole thing is unnecessary.

zsparal · 2018-05-28T09:35:56Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+
+            public void Add(Node newNode)
+            {
+                var index = ~this.nodes.BinarySearch(newNode);


This doesn't seem correct. I think it should be along the lines of:

var index = this.nodes.BinarySearch(newNode); if (index < 0) this.nodes.Insert(~index, newNode); else this.nodes.Insert(index, newNode);

zsparal · 2018-05-28T12:25:37Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+
+            public Node Pop()
+            {
+                var first = this.nodes.First();


First throws an exception if the source collection is empty, this is a bad idea. I would just do:

var result = this.nodes[0]; this.nodes.RemoveAt(0); return result;

zsparal · 2018-05-28T12:28:42Z

chapters/data_compression/huffman/code/cs/HuffmanCoding.cs

+                    currentNode = currentNode.RightChild;
+
+                // Check if it's a leaf node.
+                if (currentNode.Key.Count() == 1)


You should really not check for leaf nodes this way. It works when the alphabet of whatever you want to encode consists of single characters. You can also run Huffman coding on words, in which case this would faily miserably. The nice solution is to add something like this to Node:

public IsLeaf => LeftChild == null && RightChild == null; // Then you can just do if (currentNode.IsLeaf) { // ... }

…y we're not using something like a BitArray. Change the name of the enumerator "boolean" to "bit". Fix the Pop method by using "this.nodes[0]" instead of "this.nodes.First()". Improve the way to find out, if a Node is a Leaf.

…ty bobbity". Remove unnecessary braces.

june128 · 2018-05-29T19:39:39Z

Ok, everything should be done here now.

But I may need to change the mention, so the merging needs to be delayed a bit. @Gustorn did a great job here again and without him the code would be worse. So I don't know if I should just write "with great help by x" in the files, since the help was big or just "with help by x", so things don't get subjective.
I'm also thinking about writing "reviewed by x". This would give it a whole other touch, which would maybe be quite nice.
What do you guys think?

leios · 2018-05-29T21:22:17Z

I think it's a good idea to mention who reviewed it, so //reviewed by Gustorn makes sense.

zsparal · 2018-05-29T21:38:49Z

I'm not terribly attached to having contributor comments on the top of the files. It's clear from the git history and from the PRs. If someone wants to give some credit, I think something like: // submitted by [X], thanks to [A], [B], [C] is more than enough.

june128 · 2018-05-29T21:40:21Z

I think, I'll go for // submitted by [me], thanks to [others] for the help then. Thanks for the input!

june128 · 2018-05-29T21:51:11Z

Should be done now. [put in party emoji here I guess] :)

Add Huffman in C#.

85424b9

zsparal reviewed Apr 21, 2018

View reviewed changes

june128 added 2 commits May 7, 2018 01:49

Rename EncodeResult to EncodingResult. Remove lines left over from de…

d91ea9e

…bugging. Add mention.

zsparal reviewed May 28, 2018

View reviewed changes

june128 added 3 commits May 29, 2018 01:59

Make dictionary creation recursive.

233962f

Make HuffmanCoding non-static. Fix Add method. Change input to "bibbi…

ff95d09

…ty bobbity". Remove unnecessary braces.

Use new mention template.

2f79bb5

Merge branch 'master' into addHuffmanCSharpPR

d2000db

zsparal approved these changes May 29, 2018

View reviewed changes

leios merged commit 482b1fa into algorithm-archivists:master May 29, 2018

june128 deleted the addHuffmanCSharpPR branch May 29, 2018 21:57

Uh oh!

Add Huffman in C#. #82

Add Huffman in C#. #82

Uh oh!

Conversation

june128 commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ToDo

Uh oh!

Choose a reason for hiding this comment

Uh oh!

june128 Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zsparal Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zsparal Apr 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zsparal Apr 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

june128 commented Apr 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

june128 May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

june128 commented Apr 19, 2018 •

edited

Loading

june128 Apr 22, 2018 •

edited

Loading

zsparal Apr 22, 2018 •

edited

Loading

zsparal Apr 21, 2018 •

edited

Loading

zsparal Apr 21, 2018 •

edited

Loading

june128 May 28, 2018 •

edited

Loading

zsparal May 28, 2018 •

edited

Loading

zsparal May 28, 2018 •

edited

Loading

june128 commented May 29, 2018 •

edited

Loading