0
\$\begingroup\$

I want to create a file consisting take rows of distinct numbers in ascending order. They are randomly taken from the first total integer. The file will be used to make an excerpt as discussed here.

I realize that my naming convention for identifiers below is not good but please ignore it for the sake of simplicity.

using System;
using System.IO;
using System.Linq;

namespace Karls.Students.Excerpting
{
    class Program
    {
        static void Main(string[] args)
        {
            int total = int.Parse(args[0]);
            int take = int.Parse(args[1]);
            int seeder = int.Parse(args[2]);
            string filename = args[3];


            int[] array = Enumerable.Range(1, total).ToArray();

            Random random = new Random(seeder);
            for (int i = total - 1; i > 0; i--)
            {
                int j = random.Next(i+1);
                int temp = array[i];
                array[i] = array[j];
                array[j] = temp;
            }

            File.WriteAllLines(filename, array.Take(take).OrderBy(x => x).Select(x => x.ToString()));
        }
    }
}

With best practice in mind, is there anything "bad" in my code?

\$\endgroup\$
  • \$\begingroup\$ In other words: How to make an array (of type int with length m) containing distinct, ascending ordered numbers in a set of the first n positive integers (n>m)? The difference between two consecutive elements in the array in question must be a random number (not necessarily distinct). \$\endgroup\$ – kiss my armpit Apr 2 '13 at 11:18
3
\$\begingroup\$

Instead of shuffling items yourselves it's easier to apply random sort. So your logic can be rewritten in a single statement:

var random = new Random();

var result = Enumerable.Range(1, total)
    .OrderBy(i => random.Next())
    .Take(take)
    .OrderBy(i => i);
\$\endgroup\$
  • \$\begingroup\$ I'm ordering by random value, not selecting random values. OrderBy(i => random.Next()) just assigns a random "weight" to each number in sequence 1..total and reorders them correspondingly. The output will correspond to your requirements, take unique elements in range 1..total in ascending order. Before -1'ing the answer please try to understand it first. \$\endgroup\$ – almaz Apr 4 '13 at 9:04
  • \$\begingroup\$ What algorithm is used in random sort? fisher-yates shuffle? \$\endgroup\$ – kiss my armpit Apr 4 '13 at 9:15
  • \$\begingroup\$ Fisher-Yates shuffle is the one you've specified in your code. The one I specified is another commonly used shuffling method with uniform distribution, usually called as "random sort". \$\endgroup\$ – almaz Apr 4 '13 at 9:42
2
\$\begingroup\$

With out commenting on the business logic (the class assignment) I would say this about the coding standard as a whole.

My opionion is: You are writing very c style. Instead think of C# as oo

So Smaller functions, this make it very obvious to anyone reading it whats going on with out trying to understand the actually code. Easier to read = easier to maintain and extend project later on..

Your primary function (usually the public method) should read like an english book.

Also you should check as soon as posible for negetive cases, in theory on top of the function. This helps nesting and mid code aborts.

So with a quick style rewrite (sorry not going to touch the business) something like:

namespace Karls.Students.Excerpting
{
   class Program
   {
    static void Main(string[] args)
    {
        var commandLineArgs = new CommandLineArgs(args);

        var array = CreatePlaceHolderStorage(commandLineArgs.Total);

        Randomize(commandLineArgs.Seeder, array);

        WriteListToFile(array, commandLineArgs.Filename, commandLineArgs.Take);
    }

    private static void Randomize(int seed, int[] array)
    {
        var random = new Random(seed);
        for (int i = array.Count() - 1; i > 0; i--)
        {
            int j = random.Next(i + 1);
            int temp = array[i];
            array[i] = array[j];
            array[j] = temp;
        }
    }

    private static int[] CreatePlaceHolderStorage(int total)
    {
        return Enumerable.Range(1, total).ToArray();
    }


    private static void WriteListToFile(IEnumerable<int> listToPrint, string filename, int sizeToTake)
    {
        File.WriteAllLines(filename, listToPrint.Take(sizeToTake).OrderBy(x => x).Select(x => x.ToString()));
    }
}

public class CommandLineArgs
{
    public int Total { get; set; }
    public int Take { get; set; }
    public int Seeder { get; set; }
    public string Filename { get; set; }

    public CommandLineArgs(string[] args)
    {
        Total = int.Parse(args[0]);
        Take = int.Parse(args[1]);
        Seeder = int.Parse(args[2]);
        Filename = args[3];
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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