6
\$\begingroup\$

The algorithm gets the number associated with each char, then adds a random number to it and saves the numbers it added. I plan to make an Encrypter class later. Here is the entire project:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Diagnostics;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using System.Net;
using System.Net.NetworkInformation;
using System.Net.Sockets;
using System.Threading;
using System.Text.RegularExpressions;
using WindowsInput;

using System.IO;

namespace pl
{
public partial class Form1 : Form
{
    public Form1()
    {
        InitializeComponent();
    }
    Thread tr;
    string rr;
    string r;
    char[] cr;
    int[] keys;
    int[] more;
    char[] c;
    string filePath;
    string pass;
    string en = "";
    public delegate void functionpointer(int print);
    public delegate void functionpointerr();

    private void payl()
    {
        en = pass;
        Random ra = new Random();
        this.keys = new int[en.Length];
        int ac = 0;
        c = en.ToCharArray();
        for (int i = 0; i < en.Length; i++)
        {
            functionpointer b = new functionpointer(pk);
            keys[i] = ra.Next(0, 30000);
            Invoke(b, keys[i]);
            int agfs = (int)c[i] + keys[i];
            ac = agfs;
            try
            {
                c[i] = Convert.ToChar(ac);
            }
            catch
            {
                MessageBox.Show("The text is way too long!"); // fix this this will keep iterating lmfao
                richTextBox1.Text = "";
                agfs = 0;
            }
        }
        this.r = new string(c); // Convert the Encrypted Code into a string so we can display it in RichTextbox.
        functionpointerr bb = new functionpointerr(lol);
        Invoke(bb);
    }

    public void pk(int print)
    {
        richTextBox3.Text = richTextBox3.Text + " " + print;
    }

    public void lol()
    {
        richTextBox1.Text = this.r;
    }

    private void button1_Click(object sender, EventArgs e) // Encrypt Button
    {
        pass = richTextBox1.Text;
        Thread t = new Thread(payl);
        t.Start();
    }

    private void button2_Click(object sender, EventArgs e) // Decrypt Button
    {
        en = richTextBox1.Text;
        c = richTextBox1.Text.ToCharArray();
        string fix = richTextBox3.Text;
        char[] SPACEchecker = fix.ToCharArray(); //BUY EGGS ON WEDNESDAY! 
        if (SPACEchecker[0] == ' ')
        {
            fix = fix.Remove(0, 1);
            richTextBox3.Text = fix;
        }
        Decrypt();
    }

    private void Decrypt()
    {
        int ac = 0;
        cr = new char[en.Length];
        string[] RIA = Regex.Split(richTextBox3.Text.ToString(), " ");
        int[] NKeys = new int[RIA.Length];
        //converting keys
        for (int cn = 0; cn < RIA.Length; cn++)
        {
            string it = RIA[cn].ToString();
            NKeys[cn] = int.Parse(it);
        }

        for (int p = 0; p < en.Length; p++)
        {
            cr[p] = this.c[p]; // Make cr the same as c.
        }
        for (int b = 0; b < en.Length; b++)
        {
            int agfs = (int)this.c[b] - NKeys[b];
            ac = agfs;
            try
            {
                cr[b] = Convert.ToChar(ac);
            }
            catch
            {
                MessageBox.Show("The text is way too long!"); // fix later
                richTextBox2.Text = "";
                agfs = 0;
            }
        }
        this.rr = new string(cr);
        richTextBox2.Text = this.rr;
    }

    private bool even(int numm)
    {
        if (numm % 2 == 0)
        {
            return true;
        }
        else
        {
            return false;
        }
    }

    private void button3_Click(object sender, EventArgs e)
    {
        try
        {
            OpenFileDialog o = new OpenFileDialog();
            DialogResult = o.ShowDialog();
            filePath = o.FileName;
            string text = File.ReadAllText(filePath);
            pass = text;
            tr = new Thread(payl);
            tr.Start();
            Thread ch = new Thread(check);
            ch.Start();
        }
        catch (Exception ex)
        {
            MessageBox.Show("There was a problem \n technical details " + ex);
        }
    }

    private void check()
    {
        while (tr.IsAlive)
        {
            Thread.Sleep(100);
        }
        File.Delete(filePath);
        string encryptedtext = new string(c);
        File.WriteAllText(filePath, encryptedtext);
    }

    private void button4_Click(object sender, EventArgs e)
    {
        this.rr = new string(cr);
        string fix = richTextBox3.Text;
        char[] SPACEchecker = fix.ToCharArray();
        if (SPACEchecker[0] == ' ')
        {
            fix = fix.Remove(0, 1);
            richTextBox3.Text = fix;
        }
        try
        {
            OpenFileDialog o = new OpenFileDialog();
            DialogResult = o.ShowDialog();
            filePath = o.FileName;
            richTextBox1.Text = File.ReadAllText(filePath);
            c = richTextBox1.Text.ToCharArray();
            en = richTextBox1.Text;
            Decrypt();
            File.Delete(filePath);
            File.WriteAllText(filePath, this.rr);
        }
        catch (Exception ex)
        {
            MessageBox.Show("There was a problem \n technical details " + ex);
        }
    }

}
}
\$\endgroup\$
3
  • \$\begingroup\$ I'm going to be honest - in a 60-second passover of your code, I see nothing salvageable; inappropriate naming, useless comments, terrible organization, and frankly entirely unreadable. You mention that you're going to make an Encrypter class later - I would suggest doing organizational tasks like that before implementing anything at all. You're not benefitting by coding everything then having to reorganize later, because you have to update half your code to handle the new architecture. It's especially a good idea to do that before posting to CodeReview :) \$\endgroup\$ Commented Jul 22, 2015 at 15:26
  • 2
    \$\begingroup\$ I'm sorry, this reads like an April Fool's joke. Which it would be, if this were April 1st. Short, useless names, default UI element names, and so on, are all terrible. I would also suggest that you don't bother trying to actually implement your own encryption solution "for real." I know geniuses (literal, as defined by IQ), that wouldn't try to write a new encryption algorithm. It takes more than just just general intelligence to write a good algorithm. It takes years of study, practice, etc. \$\endgroup\$
    – phyrfox
    Commented Jul 23, 2015 at 0:06
  • \$\begingroup\$ 1) You wouldn't need to store each individual number, even if Random was usable for cryptography (it's not) - just the seed value, which would allow you to regenerate the sequence on-the-fly. 2) Normally when you do something like this, you restrict the random range to your input range (eg, all letters, numbers, etc), add and then apply mod (% - or a couple of other options, and this usually operates on the byte level, too). For one thing, it prevents single-possibility results; if your current program outputs 0, there's only one possible input. \$\endgroup\$ Commented Jul 23, 2015 at 12:50

3 Answers 3

27
\$\begingroup\$

Naming

public void lol()
{
    richTextBox1.Text = this.r;
}

C# isn't LOLCODE, and shouldn't have functions named lol, ever. Use descriptive and meaningful names.

Most of your names are pretty bad:

public void pk(int print)
string rr;
string r;
namespace pl
class Form1 : Form

Additionally, this is named check():

private void check()
{
    while (tr.IsAlive)
    {
        Thread.Sleep(100);
    }
    File.Delete(filePath);
    string encryptedtext = new string(c);
    File.WriteAllText(filePath, encryptedtext);
}

It is doing a LOT more than checking something.

even(int numm)

That should be named IsEven(int num)


if (true)

This gives me the shivers:

if (numm % 2 == 0)
{
    return true;
}
else
{
    return false;
}

Just do return numm % 2 == 0;.


Assigning values to a type?

DialogResult = o.ShowDialog();

That doesn't do anything. In fact, I'd be a bit surprised if it even compiles. Just call o.ShowDialog();.


Excessive loops:

I am pretty sure you can combine these loops:

for (int p = 0; p < en.Length; p++)
{
    cr[p] = this.c[p]; // Make cr the same as c.
}
for (int b = 0; b < en.Length; b++)
{
    int agfs = (int)this.c[b] - NKeys[b];
    ac = agfs;
    try
    {
        cr[b] = Convert.ToChar(ac);
    }
    catch
    {
        MessageBox.Show("The text is way too long!"); // fix later
        richTextBox2.Text = "";
        agfs = 0;
    }
}

This should do it:

for (int b = 0; b < en.Length; b++)
{
    cr[b] = this.c[b]; // Make cr the same as c.

    int agfs = (int)this.c[b] - NKeys[b];
    ac = agfs;
    try
    {
        cr[b] = Convert.ToChar(ac);
    }
    catch
    {
        MessageBox.Show("The text is way too long!"); // fix later
        richTextBox2.Text = "";
        agfs = 0;
    }
}

Comments:

cr[b] = this.c[b]; // Make cr the same as c.

It is pretty obvious what you are doing without the comment.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ DialogResult = o.ShowDialog(); Thanks for catching that! \$\endgroup\$
    – ponoodle
    Commented Jul 22, 2015 at 3:07
  • \$\begingroup\$ @ponoodle I haven't studied encryption, so it will be safer (literally) if I leave that aspect for other reviewers to cover. \$\endgroup\$
    – user34073
    Commented Jul 22, 2015 at 4:26
  • 17
    \$\begingroup\$ @ponoodle I think you can get an effective review of coding styles here, but encryption generally comes with the caveat of "don't do it." Always rely on high quality libraries to do encryption. There are a mindboggling number of things that can go wrong with encryption. There are so many things, in fact, that it is often difficult to even get someone to bother looking at it from a cryptographic perspective, under the presumption "it'll never be secure because you wrote it yourself." \$\endgroup\$
    – Cort Ammon
    Commented Jul 22, 2015 at 6:40
  • 5
    \$\begingroup\$ To give you a rather subjective sense of perspective on this: I think it would be reasonable to claim that every single line of an acceptable quality crypto algorithm has gone through more peer review than many companies put into their entire product line in a decade. \$\endgroup\$
    – Cort Ammon
    Commented Jul 22, 2015 at 6:41
  • 1
    \$\begingroup\$ As an example, your "encryption" adds a random value from 0-30000 to a character (whose value will be generally 0-128). This means the output contains "mostly randomness." Roughly 99.5% of the content of the file is random data. Unfortunately, C#'s Random class does not have any cryptographic guarantees. This means it may be trivial to reconstruct your key, simply by looking at enough bytes of text. To pick an example, if you ever get 624 contiguous values from a Mersene Twister, you can regenerate the key. \$\endgroup\$
    – Cort Ammon
    Commented Jul 22, 2015 at 6:47
25
\$\begingroup\$

Since you asked for a feedback on the encryption algorithm, too.

You are not specialized in cryptography.

Do not even attempt to come up with a cryptographic algorithm unless you're doing it for fun and/or to learn and plan to never actually use it.

As Bruce Schneier said:

Anyone, from the most clueless amateur to the best cryptographer, can create an algorithm that he himself can't break. It's not even hard. What is hard is creating an algorithm that no one else can break, even after years of analysis. And the only way to prove that is to subject the algorithm to years of analysis by the best cryptographers around.

As Philip Zimmermann said:

When I was in college in the early seventies, I devised what I believed was a brilliant encryption scheme. A simple pseudorandom number stream was added to the plaintext stream to create ciphertext. This would seemingly thwart any frequency analysis of the ciphertext, and would be uncrackable even to the most resourceful government intelligence agencies. I felt so smug about my achievement.

Years later, I discovered this same scheme in several introductory cryptography texts and tutorial papers. How nice. Other cryptographers had thought of the same scheme. Unfortunately, the scheme was presented as a simple homework assignment on how to use elementary cryptanalytic techniques to trivially crack it. So much for my brilliant scheme.

From this humbling experience I learned how easy it is to fall into a false sense of security when devising an encryption algorithm. Most people don't realize how fiendishly difficult it is to devise an encryption algorithm that can withstand a prolonged and determined attack by a resourceful opponent. Many mainstream software engineers have developed equally naive encryption schemes (often even the very same encryption scheme), and some of them have been incorporated into commercial encryption software packages and sold for good money to thousands of unsuspecting users.

If your goal in asking for feedback here is to know if your algorithm is secure, the answer is: No. Use a reputable library.

\$\endgroup\$
4
  • \$\begingroup\$ I agree with you completely, but this should be a comment, not an answer, in my opinion. Granted, you can't have those huge quotes in a comment, but I think your point would come across anyway. \$\endgroup\$ Commented Jul 22, 2015 at 14:29
  • 1
    \$\begingroup\$ I'm curious how that encryption scheme Philip came up with is trivially cracked. Is it just a matter of figuring out the seed for the random number generator? \$\endgroup\$ Commented Jul 22, 2015 at 22:14
  • 1
    \$\begingroup\$ @ChrisCirefice I know, it's borderline. But it would be impossible to post it as a (single) comment, plus he asked specifically for a review of his algorithm too, so… \$\endgroup\$
    – Albireo
    Commented Jul 23, 2015 at 6:16
  • 1
    \$\begingroup\$ @Sohcahtoa82 thou asketh and thou shall receive: security.stackexchange.com/q/6740/7746 (Sorry if I misspelled ye olde English.) \$\endgroup\$
    – Albireo
    Commented Jul 23, 2015 at 6:24
15
\$\begingroup\$

I know that Hosch250 has already covered naming but I think it's worth letting you in on a secret...

Most programmers would prefer "stupid" code that is well written, with good naming and consistent formatting to "clever" code with ridiculous names.

I don't want to bash you too hard but button3? I bet you my house, wife and dog that you won't have a clue what that button does next week just by looking at this code.

I worked with a brilliant "stupid" coder a few years ago and something he told me really stuck.

When adding a control to a form, if you can't name it to make it clear what it does you haven't thought enough about the design of the form.

So firstly, read style guidelines and the naming guidelines.

I'm not going to give any specific guidance here because I'll be here all day. Please read the guides.


Thread synchronisation

It's generally one of the more difficult parts of programming and this is just wrong:

while (tr.IsAlive)
{
    Thread.Sleep(100);
}

If you're waiting for the other thread to finish before you start there's no point in starting two threads - you've got sequential processing there and don't need (and can't) parallelize it.

You could use a Task with a coninuation (.ContinueWith). You could start the second piece of work immediately after the first bit has finished. You could throw away all of the threading (preferred).

string rr;
string r;

WHAT?!?!

Sorry, I got distracted.

You don't need to define these delegates:

public delegate void functionpointer(int print);
public delegate void functionpointerr();

We all ready have the (well named) Action and Action<T> delegates for that.

Comments

I know that as programmers we're all incredibly funny and brilliant people, comments can be funny. Your comments leave a lot to be desired:

MessageBox.Show("The text is way too long!"); // fix this this will keep iterating lmfao
  1. The message isn't accurate
  2. Don't end messages to the user with an exclamation mark, it makes users feel bad
  3. Your comment doesn't add anything to the code
  4. Your comment isn't grammatically correct
  5. lmfao != .
  6. If you need to come back to something, use a // todo comment so that it appears in your task list in visual studio

And this:

private void button1_Click(object sender, EventArgs e) // Encrypt Button

Comments shouldn't tell me what something is, the name of it should.

I don't even know what to say about this:

char[] SPACEchecker = fix.ToCharArray(); //BUY EGGS ON WEDNESDAY! 

The naming of the variable is terrible (spaceChecker at best). The comment is, I assume, some sort of inside joke - either that or you keep a very odd shopping list.

To be completely brutal, everything about this code screams "buggy, unprofessional, puerile".

\$\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.