Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've developed mini utility, the source code can get reviewed here:

http://rle.codeplex.com/SourceControl/BrowseLatest

It's developed in C#, in the general page there is a description. I'm listening to well criticism to make code more elegant and better to work with.

 /*
 * GNU General Public License version 2 (GPLv2) http://rle.codeplex.com/license
 * Oleg Orlov, 2013 (c), RLE encoding/decoding tool, version 1.0.1 (v1.01)
 * 
 * C#, .NET 2.0 by default. It could be upgraded to any version of the .NET framework.
 * I have downgraded the .NET version only for the compatibility aims
 * and for the easy reproduction of the program to any other language.
 * 
 * http://rle.codeplex.com/
 */

using System;

abstract class Contracts
{
    public enum EncodingFormat
    {
        Old,
        New,
    };
}

class RLE : Contracts, IDisposable
{
    private string str_base, str_rle;

    private bool HasChar(ref string input)
    {
        bool status = false;

        for (int i = 0; i < input.Length; i++)
        {
            if (Char.IsLetter(input[i]))
            {
                status = true;
                break;
            }
        }

        return status;
    }

    internal string Encode(ref string input, EncodingFormat format)
    {
        str_rle = null;
        str_base = input;

        if (format == EncodingFormat.New)
        {
            for (int i = 0; i < str_base.Length; i++)
            {
                char symbol = str_base[i];
                int count = 1;

                for (int j = i; j < str_base.Length - 1; j++)
                {
                    if (str_base[j + 1] != symbol) break;

                    count++;
                    i++;
                }

                if (count == 1) str_rle += symbol;
                else str_rle += count.ToString() + symbol;
            }
        }
        else if (format == EncodingFormat.Old)
        {
            for (int i = 0; i < str_base.Length; i++)
            {
                char symbol = str_base[i];
                int count = 1;

                for (int j = i; j < str_base.Length - 1; j++)
                {
                    if (str_base[j + 1] != symbol) break;

                    count++;
                    i++;
                }

                str_rle += count.ToString() + symbol;
            }
        }

        return str_rle;
    }

    internal string Decode(ref string input)
    {
        str_rle = null;
        str_base = input;
        int radix = 0;

        for (int i = 0; i < str_base.Length; i++)
        {
            if (Char.IsNumber(str_base[i]))
            {
                radix++;
            }
            else
            {
                if (radix > 0)
                {
                    int value_repeat = Convert.ToInt32(str_base.Substring(i - radix, radix));

                    for (int j = 0; j < value_repeat; j++)
                    {
                        str_rle += str_base[i];
                    }

                    radix = 0;
                }
                else if (radix == 0)
                {
                    str_rle += str_base[i];
                }
            }
        }

        if (str_rle == null || !HasChar(ref str_rle)) throw new Exception("\r\nCan't to decode! Input string has the wrong syntax. There isn't any char (e.g. 'a'->'z') in your input string, there was/were only number(s).\r\n");

        return str_rle;
    }

    internal double GetPercentage(double x, double y)
    {
        return (100 * (x - y)) / x;
    }

    public void Dispose()
    {
        if (str_rle != null || str_base != null)
        {
            str_rle = str_base = null;
        }
    }
}

class Program : Contracts
{
    private static string str_welcome = "\r\nRLE encoding/decoding tool, Oleg Orlov 2013(c).",
        str_notice = "\r\nPlease, use the next syntax: <action> <string>\r\n(e.g. \"encode my_string\" or \"decode my_string\").\r\n\r\nWarning! The 2nd parameter (the string for encoding/decoding)\r\nmust not content any whitespaces!\r\n\r\nYou may also use the option \"-old\" to encode your string\r\n(e.g. \"encode my_string -old\") in such way, where before\r\nsingle char inserting the value: '1' (e.g. \"abbcddd\" -> \"1a2b1c3d\").";

    private static void EncodeString(ref string str, EncodingFormat format)
    {
        using (RLE inst_rle = new RLE())
        {
            string str_encoded = inst_rle.Encode(ref str, format);

            if (format == EncodingFormat.New)
            {
                Console.WriteLine("\r\nBase string ({0} chars): {1}\r\nAfter RLE-encoding ({2} chars): {3}\r\nCompression percentage: %{4}",
                                    str.Length, str, str_encoded.Length, str_encoded,
                                    inst_rle.GetPercentage((double)str.Length, (double)str_encoded.Length).ToString());
            }
            else if (format == EncodingFormat.Old)
            {
                Console.WriteLine("\r\nBase string ({0} chars): {1}\r\nAfter RLE-encoding with the \"-old\" option ({2} chars): {3}\r\nCompression percentage: %{4}",
                    str.Length, str, str_encoded.Length, str_encoded,
                    inst_rle.GetPercentage((double)str.Length, (double)str_encoded.Length).ToString());
            }
        }
    }

    private static void DecodeString(ref string str)
    {
        using (RLE inst_rle = new RLE())
        {
            string str_decoded = inst_rle.Decode(ref str);
            Console.WriteLine("\r\nBase string ({0} chars): {1}\r\nAfter RLE-decoding ({2} chars): {3}\r\nDecompression percentage: %{4}",
                    str.Length, str, str_decoded.Length, str_decoded,
                    Math.Abs(inst_rle.GetPercentage((double)str.Length, (double)str_decoded.Length)).ToString());
        }
    }

    [STAThread]
    public static void Main(string[] args)
    {
        try
        {
            Console.WriteLine(str_welcome);

            if (args.Length > 1)
            {
                if (args[0] == "encode")
                {
                    if (args.Length == 3)
                    {
                        if (args[2] == "-old")
                        {
                            EncodeString(ref args[1], EncodingFormat.Old);
                        }
                    }
                    else
                    {
                        EncodeString(ref args[1], EncodingFormat.New);
                    }
                }
                else if (args[0] == "decode")
                {
                    DecodeString(ref args[1]);
                }
                else
                {
                    throw (new Exception("\r\nThere are only two methods: encode (with the \"-old\" option), decode. No other actions are available.\r\n"
                        + str_notice + "\r\n"));
                }
            }
            else
            {
                Console.WriteLine(str_notice);
                Environment.Exit(1);
            }
        }
        catch (Exception exc)
        {
            Console.WriteLine("\r\n{0}", exc);
            Environment.Exit(1);
        }
    }
}
share|improve this question
    
If you want us to review your code, you need to post it here directly, not just link to it. –  svick Jan 8 '13 at 1:32
    
@svick have done! –  Ivica8080 Jan 8 '13 at 2:07
add comment

3 Answers

MPEG?

I'm a little unclear where RLE comes into the MPEG codec, but if it does I would assume its based on binary encoding, not text based? In the same way as RLE formats for BMP's?

Streaming

Anything that processes information like this should attempt to keep its memory profile to a minimum. If this is to be used for MPEG data then the input data could be huge. As the algorthum is forward only (ie you don't need to ammend information you have already written), you can make it stream based. The class would operate on a source stream (or TextReader) and write the encoded data to an output stream (or TextWriter). This way virtually no state information needs to be held and the memory footprint is virtually nill regardless of the size of the data being encoded. In any case there is no need to store the encoded/raw data.

Points about the code.

You don't need to pass by reference unless your going to change the string (ref string input). I'm guessing you were a C++ programmer?

Building string using

str += "stuff";

is very slow. It causes a re-allocation and copy. Use StringBuilder.

Throwing Exception is bad, you should use an existing .Net Exception or derive your own. It means code has to have catch(Exception) which causes ThreadAbortException to be caught (which is not typically what you want to happen). There is a fair bit of material out there about Exception throwing.

GetPercentage is a helper function, its nothing to do with the encoder, so it should not live in the encoder class.

Dispose is for freeing up unmanaged resources, if you insist on storing the raw/encoded data then you may want a method to cleanup method for discarding the data, but dispose has no practical use here. Read up on garbage collection (If you do have a C++ background then a bit of background will prevent you writting lots of unessessary code).

Hope this helps.

share|improve this answer
    
About RLE in MPEG-2, look here: link –  Ivica8080 Jan 8 '13 at 20:21
    
I want to use Dispose for use using {} it requires to implement IDisposable –  Ivica8080 Jan 8 '13 at 20:24
    
Still pretty sure the rle implementation will be a binary one, not text based, but I've not read the spec. You only need to dispose for unmanaged resources, so in this case its not required, and therefore neither is the use of a using block. –  Sprotty Feb 5 '13 at 7:51
add comment

First of all. Follow the guidelines for .NET. Your naming convention is off. Same goes for the indention (also discussed in the same web page, but a different section).


private bool HasChar(ref string input)
{
    bool status = false;

    for (int i = 0; i < input.Length; i++)
    {
        if (Char.IsLetter(input[i]))
        {
            status = true;
            break;
        }
    }

    return status;
}

that method can be replaced with input.Any(x => char.IsLetter(x)). If you want to stick with a method, why don't you just use return true; inside the loop? Makes the code a lot cleaner.


.NET takes care of all memory management for you. The GC is quite aggressive. Implementing IDisposable just to set things to null does virtually nothing.

share|improve this answer
add comment

This looks like some sort of direct port from C. It totally ignores C# convention, standards and the .NET library that you have at your disposal. There also seems to be a bit of a misunderstanding of how inheritance works (or, in this case, isn't needed), the Disposable pattern, when instance variables versus locals should be used, use of the ref keyword and immutable string concatenation vs. StringBuilder. So, look those up in the appropriate places, learn them thoroughly and compare with this version (note, internal logic for creating the RLE itself is pretty solid, so I left it alone):

 /*
 * GNU General Public License version 2 (GPLv2) http://rle.codeplex.com/license
 * Oleg Orlov, 2013 (c), RLE encoding/decoding tool, version 1.0.1 (v1.01)
 * 
 * C#, .NET 2.0 by default. It could be upgraded to any version of the .NET framework.
 * I have downgraded the .NET version only for the compatibility aims
 * and for the easy reproduction of the program to any other language.
 * 
 * http://rle.codeplex.com/
 */

using System;
using System.Globalization;
using System.Text;

public enum EncodingFormat
{
    Old,

    New,
}

public static class Rle
{
    private static bool HasChar(StringBuilder input)
    {
        for (var i = 0; i < input.Length; i++)
        {
            if (char.IsLetter(input[i]))
            {
                return true;
            }
        }

        return false;
    }

    internal static string Encode(string input, EncodingFormat format)
    {
        var runLengthEncodedString = new StringBuilder();
        var baseString = input;

        switch (format)
        {
            case EncodingFormat.New:
                for (var i = 0; i < baseString.Length; i++)
                {
                    var symbol = baseString[i];
                    var count = 1;

                    for (var j = i; j < baseString.Length - 1; j++)
                    {
                        if (baseString[j + 1] != symbol)
                        {
                            break;
                        }

                        count++;
                        i++;
                    }

                    if (count == 1)
                    {
                        runLengthEncodedString.Append(symbol);
                    }
                    else
                    {
                        runLengthEncodedString.Append(count.ToString(CultureInfo.InvariantCulture) + symbol);
                    }
                }

                break;
            case EncodingFormat.Old:
                for (var i = 0; i < baseString.Length; i++)
                {
                    var symbol = baseString[i];
                    var count = 1;

                    for (var j = i; j < baseString.Length - 1; j++)
                    {
                        if (baseString[j + 1] != symbol)
                        {
                            break;
                        }

                        count++;
                        i++;
                    }

                    runLengthEncodedString.Append(count.ToString(CultureInfo.InvariantCulture) + symbol);
                }

                break;
        }

        return runLengthEncodedString.ToString();
    }

    internal static string Decode(string input)
    {
        var runLengthEncodedString = new StringBuilder();
        var baseString = input;

        var radix = 0;

        for (var i = 0; i < baseString.Length; i++)
        {
            if (char.IsNumber(baseString[i]))
            {
                radix++;
            }
            else
            {
                if (radix > 0)
                {
                    var valueRepeat = Convert.ToInt32(baseString.Substring(i - radix, radix));

                    for (var j = 0; j < valueRepeat; j++)
                    {
                        runLengthEncodedString.Append(baseString[i]);
                    }

                    radix = 0;
                }
                else if (radix == 0)
                {
                    runLengthEncodedString.Append(baseString[i]);
                }
            }
        }

        if (!HasChar(runLengthEncodedString))
        {
            throw new Exception("\r\nCan't to decode! Input string has the wrong syntax. There isn't any char (e.g. 'a'->'z') in your input string, there was/were only number(s).\r\n");
        }

        return runLengthEncodedString.ToString();
    }

    internal static double GetPercentage(double x, double y)
    {
        return (100 * (x - y)) / x;
    }
}

internal static class Program
{
    private const string Welcome = "\r\nRLE encoding/decoding tool, Oleg Orlov 2013(c).";

    private const string Notice = "\r\nPlease, use the next syntax: <action> <string>\r\n(e.g. \"encode my_string\" or \"decode my_string\").\r\n\r\nWarning! The 2nd parameter (the string for encoding/decoding)\r\nmust not content any whitespaces!\r\n\r\nYou may also use the option \"-old\" to encode your string\r\n(e.g. \"encode my_string -old\") in such way, where before\r\nsingle char inserting the value: '1' (e.g. \"abbcddd\" -> \"1a2b1c3d\").";

    private static void EncodeString(string unencodedString, EncodingFormat format)
    {
        var encodedString = Rle.Encode(unencodedString, format);

        switch (format)
        {
            case EncodingFormat.New:
                Console.WriteLine(
                    "\r\nBase string ({0} chars): {1}\r\nAfter RLE-encoding ({2} chars): {3}\r\nCompression percentage: %{4}",
                    unencodedString.Length,
                    unencodedString,
                    encodedString.Length,
                    encodedString,
                    Rle.GetPercentage(unencodedString.Length, encodedString.Length));
                break;
            case EncodingFormat.Old:
                Console.WriteLine(
                    "\r\nBase string ({0} chars): {1}\r\nAfter RLE-encoding with the \"-old\" option ({2} chars): {3}\r\nCompression percentage: %{4}",
                    unencodedString.Length,
                    unencodedString,
                    encodedString.Length,
                    encodedString,
                    Rle.GetPercentage(unencodedString.Length, encodedString.Length));
                break;
        }
    }

    private static void DecodeString(string encodedString)
    {
        var decodedString = Rle.Decode(encodedString);

        Console.WriteLine(
            "\r\nBase string ({0} chars): {1}\r\nAfter RLE-decoding ({2} chars): {3}\r\nDecompression percentage: %{4}",
            encodedString.Length,
            encodedString,
            decodedString.Length,
            decodedString,
            Math.Abs(Rle.GetPercentage(encodedString.Length, decodedString.Length)));
    }

    [STAThread]
    public static int Main(string[] args)
    {
        try
        {
            Console.WriteLine(Welcome);

            if (args.Length > 1)
            {
                switch (args[0])
                {
                    case "encode":
                        if (args.Length == 3)
                        {
                            if (args[2] == "-old")
                            {
                                EncodeString(args[1], EncodingFormat.Old);
                            }
                        }
                        else
                        {
                            EncodeString(args[1], EncodingFormat.New);
                        }

                        break;
                    case "decode":
                        DecodeString(args[1]);
                        break;
                    default:
                        throw new Exception("\r\nThere are only two methods: encode (with the \"-old\" option), decode. No other actions are available.\r\n"
                                             + Notice + "\r\n");
                }

                return 0;
            }

            Console.WriteLine(Notice);
            return 1;
        }
        catch (Exception exc)
        {
            Console.WriteLine("\r\n{0}", exc);
            return 1;
        }
    }
}
share|improve this answer
    
Thank you for the such good answer. But I have Q about your code. Why is var needed? As I remember var makes some more actions for runtime type identification? So maybe to explicitly use the exact type and not using var? –  Ivica8080 Jan 8 '13 at 20:42
    
var is not needed at all in this instance. It's a matter of preference. It has nothing to do with RTTI. The compiler resolves its type at compile-time. –  Jesse C. Slicer Jan 8 '13 at 20:45
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.