4
\$\begingroup\$

I am trying to encrypt the input string using the following rule:

  • 1st Character of string will be increment by 4 ( a will become e )
  • 2nd Character of string will be increment by 1 ( a will become b )
  • 3rd Character of string will be increment by 7 ( a will become h )
  • Rest of the characters will be replaced using the above rules.

Examples:

(Before Enc)    (After Enc)
""              ""         
"b"             "f"       
"x"             "b"     
"imagine"       "mnhkjui"
"abcdef"        "ecjhfm"
"xz"            "ba"    

I have written the following code to achieve this:

public static string Puzzle(string str) 
{
    string result = "";
    var w = s.a;
    string x = "";
    foreach (var item in str)
    {
        if (item + (int)w > 122)
        {
            x += (char)(96 + ((item + (int)w) % 122));
        }
        else
        {
            x += (char)((int)w + item);
        }
        w = Convert(w);
    }
    return x;
}

enum s { a = 4, b = 1, c = 7 };

private static s Convert(s w)
{
    if (w == s.a)
    {
        w = s.b;
    }
    else if (w == s.b)
    {
        w = s.c;
    }
    else
    {
        w = s.a;
    }
    return w;
}

What I want is a better and more efficient solution than the one I have written.

\$\endgroup\$
7
  • \$\begingroup\$ More efficient in what sense? \$\endgroup\$
    – Ant P
    Commented Jan 18, 2015 at 20:21
  • \$\begingroup\$ more efficient in smart code, faster and just better than this... \$\endgroup\$ Commented Jan 18, 2015 at 20:23
  • \$\begingroup\$ Can you post code that compiles first? Where is x declared? \$\endgroup\$
    – Ant P
    Commented Jan 18, 2015 at 20:47
  • 1
    \$\begingroup\$ Your code does not compile. Like I said, where is x declared? What is it supposed to be? Should x be result? \$\endgroup\$
    – Ant P
    Commented Jan 18, 2015 at 20:48
  • 1
    \$\begingroup\$ Just so it's said: this isn't encryption. Encryption requires a key. At best, this is encoding. \$\endgroup\$
    – cHao
    Commented Jan 19, 2015 at 13:33

2 Answers 2

5
\$\begingroup\$

Your Convert function cycles through the values 4, 1, 7. You do this by comparing the given state with each possible state, and then return the next state. This could be made much simpler by using an index rather than values. Then instead of calling Convert, we can increment the index and if necessary wrap the index around by using a modulo operation. When we need the value, we can use the index to look it up in an array that contains these values.

This would look similar to:

int[] states = {4, 1, 7};
int i = 0;

// get the value:
states[i]

// increment the index:
i = (i + 1) % states.Length;

If we really want to go overboard with this, we could define a type that represents these cyclical states:

class Cycle<T>
{
    private readonly T[] values;
    private int i = 0;

    public Cycle(params T[] values)
    {
        this.values = values;
    }

    public T Value
    {
        get
        {
            return values[i];
        }

    }

    public static Cycle<T> operator++ (Cycle<T> cycle)
    {
        cycle.i = (cycle.i + 1) % cycle.values.Length;
        return cycle;
    }
}

Used as:

var states = new Cycle<int>(4, 1, 7);

// get the value:
states.Value

// increment the index:
++states;

While using such a class makes for a very nice interface, it's not exactly the most efficient thing to do. Using an array as shown at the top is probably a good compromise between good code and performance. In particular, both of my solutions allow you to set the values of the cycle at runtime, rather than hardcoding all values in an enum.

\$\endgroup\$
0
3
\$\begingroup\$

In addition to the comments in amon's answer I'd suggest you to change the following things:

  • change the type of result to StringBuilder instead of a string. It is more efficient.
  • try to use meaningful names. I think it would be clearer if you would call your variables in a different way. For instance, I'd use offset in place of w. Wouldn't it be clearer?

Looking at the code, I'd introduce some functions to make it clearer. Using Cycle as described in amon's answer it's great. I'd also factor out the code to compute the encrypted value.

var outputBuilder = new StringBuilder();
var offsets = new Cycle<int>(4, 1, 7);
foreach (var currentCharacter in inputString)
{
    outputBuilder.Append(Encrypt(currentCharacter, offsets.Value);
    ++offsets;
}
return outputBuilder.ToString();
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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