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 in 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
3  
I guess I had better get back to typing up some Macbeth! –  rolfl Feb 25 at 18:45
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 Feb 25 at 22:50
24  
Use a profiler to answer a performance question. Anything else is guessing. –  Eric Lippert Feb 26 at 7:13
11  
@agentnega: You see what you're doing there, right? You are guessing. Your guesses are good, educated guesses that are probably right, but they are still guesses. For all we know the reason that the C# program is slow is because there's something wrong with the Console.WriteLine causing the output to block or some such thing. I have profiled a lot of C# programs in my day and very frequently -- well over 10% of the time -- my initial guess about the cause of a slowdown is utterly wrong. Engineers solve problems by reasoning about facts, not guesses. –  Eric Lippert Feb 26 at 16:12
1  
You say "I wrote it on C#, because it's compiled"; Python is also normally compiled. –  Russell Borogove Feb 27 at 3:06
show 1 more comment

5 Answers

up vote 17 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. –  bobpal Feb 25 at 19:29
5  
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 Feb 25 at 19:35
1  
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. –  bobpal Feb 25 at 20:04
    
@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 Feb 25 at 21:08
1  
@Mat'sMug Never mind, I should have written a more thorough answer :-) –  vals Feb 25 at 21:34
show 5 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 Feb 25 at 18:50
    
The objects are created once every user input, hardly a performance issue... –  Uri Agassi Feb 25 at 18:51
5  
@UriAgassi they're not. Objects are created at each iteration of the while loop, which runs after the user input. –  Mat's Mug Feb 25 at 19:40
3  
@Phoshi I think that my use of the plural reuse them make you believe that I was suggesting using a pool of objects. My suggestion was much easier; just create one object of class Random and one of class StringBuilder, and them meant the 2 objects. –  vals Feb 26 at 17:34
2  
@Andris: "Don't object pool or reuse by default, only do it when there's actually an issue". Random has semantic issues, never mind speed issues, and should never be created in a tight loop. This does not hold for the vast majority of objects, though. –  Phoshi Feb 27 at 11:20
show 6 more comments

To answer the question of why your c# code is so slow.

It's this line. According to a profiler ~90% of the execution time is being taken up by recreating the Random object. You'll notice your python code only uses the random rather than recreates it.

Random rnd = new Random();

if you change the generate method to:

    static Random rnd = new Random();

    static string generate(int len)
    {
        //generates string of random letters, returns the random string
        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();
    }

These changes will show similar perf for both languages.

share|improve this answer
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
1  
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 Feb 25 at 19:17
    
@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 Feb 25 at 19:19
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 Feb 25 at 23:43
    
@ChrisW dangit, it screws up the "best so far" display. Whoops. Correcting. –  Jesse C. Slicer Feb 25 at 23:55
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.