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 made a program on python, and wanted it to be faster, so I wrote it on C#, because it's compiled. To my surprise, the python program is much faster. I guess there is something wrong with my C# code, but it is pretty simple and straightforward so I don't know. They are structured about the same way.

C#:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Text.RegularExpressions;
using System.Diagnostics;

//This program generates a string of random lowercase letters and matches it to the user's input
//It does this until it gets a match
//It also displays the closest guess so far and the time it took to guess

namespace Monkey
{
    class Program
    {
        static string userinput()
        {
            //Takes user input, makes sure it is all lowercase letters, returns string
            string input;

            while(true)
            {
                input = Console.ReadLine();

                if (Regex.IsMatch(input, @"^[a-z]+$"))
                {
                    return input;
                }
            }
        }

        static string generate(int len)
        {
            //generates string of random letters, returns the random string
            Random rnd = new Random();
            string alpha = "abcdefghijklmnopqrstuvwxyz";
            int letterInt;
            StringBuilder sb = new StringBuilder();


            for (int i = 0; i < len; i++)
            {
                letterInt = rnd.Next(26);
                sb.Append(alpha[letterInt]);
            }

            return sb.ToString();
        }

        static int count(int len, string s, string g)
        {
            //returns number of letters that match user input
            int same = 0;

            for (int i = 0; i < len; i++)
            {
                if(g[i] == s[i])
                {
                    same++;
                }
            }

            return same;
        }

        static void Main(string[] args)
        {
            Console.WriteLine("They say if you lock a monkey in a room with a typewriter and enough time,");
            Console.WriteLine("the monkey would eventually type a work of Shakespeare.");
            Console.WriteLine("Let's see how well C# does...");
            Console.WriteLine("Enter a word");
            Console.WriteLine("(3 letters or less is recommended)");
            string solution = userinput();

            int size = solution.Length;
            bool success = false;
            string guess = null;
            int correct;
            int best = 0;
            Stopwatch watch = Stopwatch.StartNew();

            while (!success)
            {
                guess = generate(size);
                correct = count(size, solution, guess);

                if (correct == size)
                {
                    success = true;
                }

                else if (correct > best)
                {
                    Console.Write("The best guess so far is: ");
                    Console.WriteLine(guess);
                    best = correct;
                }
            }

            watch.Stop();
            TimeSpan ts = watch.Elapsed;
            Console.WriteLine("Success!");
            Console.Write("It took " + ts.TotalSeconds + " seconds for the sharp C to type ");
            Console.WriteLine("\"" + guess + "\"");

            Console.ReadLine();
        }
    }
}

Python:

import random
import time
#This program generates a string of random letters and matches it with the user's string
#It does this until it's guess is the same as the user's string
#It also displays closest guess so far and time it took to guess


def generate():
    # generate random letter for each char of string
    for c in range(size):
        guess[c] = random.choice(alpha)


def count():
    # count how many letters match
    same = 0
    for c in range(size):
        if guess[c] == solution[c]:
            same += 1
    return same


print("They say if you lock a monkey in a room with a typewriter and enough time,")
print("the monkey would eventually type a poem by Shakespeare")
print("Let's see how well a python does...'")

user = ""
badinput = True
while badinput:
    # Make sure user only inputs letters
    user = input("Enter a word\n(5 letters or less is recommended)\n")
    if user.isalpha():
        badinput = False

solution = list(user.lower())
size = len(solution)
guess = [""] * size
alpha = list("abcdefghijklmnopqrstuvwxyz")
random.seed()
success = False
best = 0    # largest number of correct letters so far
start = time.time()    # start timer

while not success:
    # if number of correct letters = length of word
    generate()
    correct = count()
    if correct == size:
        success = True
    elif correct > best:
        print("The best guess so far is: ", end="")
        print("".join(guess))
        best = correct

finish = time.time()    # stop timer
speed = finish - start

print("Success!")
print("It took " + str(speed) + " seconds for the python to type ", end="")
print("\"" + "".join(guess) + "\"")
input()
share|improve this question
2  
I guess I had better get back to typing up some Macbeth! –  rolfl 6 hours ago
1  
You may find this question on StackOverflow useful as well as it discusses pros and cons of instantiating a StringBuilder object: stackoverflow.com/questions/550702/… –  scotru 2 hours ago
add comment

4 Answers

up vote 4 down vote accepted

I don't know so I'll focus on the code.

  • Your program is coded upside down. One would expect void Main at the top, with the more specialized code below.
  • Method names in C# are expected to consistently follow PascalCasing convention. Only your Main method does that, and if you were to adopt a camelCasing convention, userinput would be better off called userInput.
  • It doesn't feel right that the Count method doesn't infer the number of iterations from the length of the string(s) it's given, and doesn't do anything to verify whether the index is legal, which at first glance seems like asking for an IndexOutOfRangeException.

Performance-wise, I think you'll need to factor out the randomness from your tests for any benchmarking to mean anything.

Using StringBuilder was a good call.

share|improve this answer
 
It is random, but if you run them both, the python is consistently much faster than the C#. Thanks for the tips. I put the methods at the bottom and changed the names. –  user2180125 5 hours ago
2  
Same if you take the creation of the rnd and sb objects outside the loop as @vals as pointed out? I suspect a seeding issue with all the instances of Random you're creating. Possibly you run the program 10K times and it runs 10K times with the same "random" sequence. Also C# is Jit-compiled from IL code, so yeah it's compiled, but to an intermediate language - there's the CLR doing its job, too. Not sure how fair your comparison is. –  Mat's Mug 5 hours ago
 
YES! the random initializing inside the method was the problem. Thanks, for pointing that out. Stepping through it in the debugger wasn't catching it. And yes, I know about C# and the way it compiles. –  user2180125 5 hours ago
 
@user2180125 in all fairness, it's really vals' answer that caught the performance issue. If you're accepting my answer for the code review, I'll take it; but if you're accepting my answer for the advice about taking instantiation out of the loop, vals' answer should have the checkmark ;) –  Mat's Mug 4 hours ago
1  
@Mat'sMug Never mind, I should have written a more thorough answer :-) –  vals 3 hours ago
show 2 more comments

If you want performance, don't create objects in the inner loop:

    static string generate(int len)
    {
        Random rnd = new Random();                    // creating a new object
        string alpha = "abcdefghijklmnopqrstuvwxyz";
        int letterInt;
        StringBuilder sb = new StringBuilder();      // creating a new object
        ....

    }

create them once and reuse them

share|improve this answer
 
Ugh. How did I miss that! Good catch! –  Mat's Mug 6 hours ago
 
The objects are created once every user input, hardly a performance issue... –  Uri Agassi 6 hours ago
2  
@UriAgassi they're not. Objects are created at each iteration of the while loop, which runs after the user input. –  Mat's Mug 5 hours ago
 
@Mat'sMug you are right, missed that :-) –  Uri Agassi 5 hours ago
 
+1 (late, I know, I ran out of ammo early today!), but despite the upvotes, this is pretty much a SO-Style answer; on CR we prefer answers that review the OP's code - not necessarily long answers redacted in 2 hours, but while addressing OP's performance concerns (for example), consider commenting on any aspect of the code, including naming, formatting, best practices, etc. That said, welcome to soon-to-be-500 milestone! –  Mat's Mug 1 hour ago
add comment

In your python code you use a character array as your guess. In your C# code you build a StringBuffer instead. Try using a char[] instead.

Since you are on Code Review - I'll also give some thoughts about your code:

Naming Conventions - C# method naming is PascalCase, and python function naming in snake_case, so - UserInput() and user_input(): respectively.

Meaningful names - generate and count do not convey well the meaning of the code in them GenerateRandomString and CountSimilarLetters is better. Same goes for alpha, len, g, s, etc.

share|improve this answer
 
You have camelCase and PascalCase confused, but at least you use them right. Though camelCase doesn't use underscores either - it just looks like a camel. –  Magus 5 hours ago
 
@Magus, fixed, thanks CamelCase can be used for both upper and lower first chars(en.wikipedia.org/wiki/CamelCase), and the pascal_case was a typo... –  Uri Agassi 5 hours ago
add comment

I'd look at compiling your regex and having it as a class-level member for optimal reuse. Also lift the random number generator out the same way, as it's never good practice to continually regenerate it.

old code:

    static string userinput()
    {
        //Takes user input, makes sure it is all lowercase letters, returns string
        string input;

        while(true)
        {
            input = Console.ReadLine();

            if (Regex.IsMatch(input, @"^[a-z]+$"))
            {
                return input;
            }
        }
    }

    private static string generate(int len)
    {
        // generates string of random letters, returns the random string
        Random rnd = new Random();
        string alpha = "abcdefghijklmnopqrstuvwxyz";
        StringBuilder sb = new StringBuilder();
        int letterInt;

        for (int i = 0; i < len; i++)
        {
            letterInt = rnd.Next(26);
            sb.Append(alpha[letterInt]);
        }

        return sb.ToString();
    }

new code:

    private static readonly Regex regex = new Regex(@"^[a-z]+$", RegexOptions.Compiled);
    private static readonly Random rnd = new Random();

    static string userinput()
    {
        //Takes user input, makes sure it is all lowercase letters, returns string
        string input;

        while(true)
        {
            input = Console.ReadLine();

            if (regex.IsMatch(input))
            {
                return input;
            }
        }
    }

    private static string generate(int len)
    {
        // generates string of random letters, returns the random string
        const string alpha = "abcdefghijklmnopqrstuvwxyz";
        StringBuilder sb = new StringBuilder();
        int letterInt;

        for (int i = 0; i < len; i++)
        {
            letterInt = rnd.Next(26);
            sb.Append(alpha[letterInt]);
        }

        return sb.ToString();
    }
share|improve this answer
1  
If the strings are 123a56 and 123b56 then your count will be 3 and the OP's count will be 5. –  ChrisW 1 hour ago
 
@ChrisW dangit, it screws up the "best so far" display. Whoops. Correcting. –  Jesse C. Slicer 1 hour ago
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.