Yes. It's possible to write this code cleanly and succinctly - and I can imagine implementing it as a single function (in an appropriately named class) or a small class with significantly more functionality.
However I don't believe in giving an answer. What I have done is provided notes about how one could improve this code, and at the end a different way of thinking about the problem. It's now your job to learn from what has been written, apply it to your work, and come up with an improved solution.
Good luck!
Any
public bool checkListLength(List<string> source)
{
return source.Count == 0;
}
This method is somewhat pointless - it's longer than the logic and not clearly named. Instead, consider using 'source.Count == 0' or an extension method such as 'source.Any()'.
Params
public string getText(List<string> source, List<string> backup_source...
This is where the 'params' keyword can come in useful:
public string getText(Random rand, params List<string>[] sources)
You can use it as such:
...getText(rand, firstList);
...getText(rand, firstList, secondList, thirdList);
Private Field
You keep passing a Random around. This creates more work for everyone, and doesn't server a clearly defined purpose. The generator should be responsible for maintaining the Random.
class text_generator
{
Random rand = new Random();
....
}
Alternatively, one could create a static Random class that handles it for the entire project.
public static class RandomManager
{
Random rand = new Random();
public static int Next(int min, int max)
{
return rand.Next(min,max);
}
....
}
Which would be used like so:
RandomManager.Next(0,10);
CamelCase
We tend to use CamelCase in C#. UpperCamelCase for Property, Method, Class and NameSpaces, and lowerCameCase for fields, parameter and variable names.
text_generator is a class, so TextGenerator.
getText is a method name so GetText.
index is a variable, so index is correct.
source is a parameter, so source is correct.
Naming
As mentioned checkListLength is a long name that doesn't describe what it does - maybe IsEmpty would have been more appropriate. You've called this text_generator... and yet it doesn't generate any text. Maybe RandomDialog might have been a better name. With this class name RandomDialog.getText(...) might be better named RandomDialog.FromLists(..).
Correctness
The code as is will throw an exception if the provided lists are null or empty... you either need to handle these statements or through a more informational guard statement yourself. You should also document this through comments.
/// <summary>
/// This function....
/// </summary>
/// <param name="sources">Sources is ... and must have a valid item to retrieve or an ArgumentException will be thrown!</param>
/// <returns>What this method returns.</returns>
public string GetText(params List<string>[] sources)
{
if (sources == null || sources.Count == 0)
throw new ArgumentException("Sources cannot be null or empty");
var listWithItems = sources.FirstOrDefault(s => s != null && s.Any());
if (listWithItems == null)
throw new ArgumentException("No items could be found in sources!");
....
}
Intent
A very important thing in programming is being able to describe your intent - which is what you want to do. From your comment:
I Generate random elements from a list without repetition until all of
them are used. Then it starts anew
This is not what your code does. It does not 'generate' random items, it retrieves items in a random order and removes them from the incoming list. It does not start anew... it crashes. You either need to restate your intent, change how your code implements this intent, or both.
Side Effects
As @RubberDuck so elegantly puts it:
A method named GetXxx shouldn't have side effects. It's surprising to
call a "Get" method and have it modify the objects you send into it.
Look into something called the Principle of Least Astonishment - it's a great way of thinking!
The principle of least astonishment (POLA) applies to user interface
and software design, from the ergonomics standpoint. It is
alternatively referred to as the law or rule of least astonishment, or
of least surprise. "If a necessary feature has a high astonishment
factor, it may be necessary to redesign the feature."
Possible Solution
Given what I think you're looking for, consider using a Queue<T>. A queue is a data structure that returns to you, in order, items in a once-off manner. Since you want to prove one or more lists, and have the results ordered in a specific manner I would create a method that does this. That is it takes in collection of List<T>s using the params keyword, and returns a queue populated according to your logic.
If you need to add some more logic - e.g. repopulate this queue once it's been finished - I would then wrap it in a class to perform this functionality. For bonus points (and for more advanced users) look into interfaces/concepts such as IEnumerable<T>/IEnumerator<T> to see if you can turn this into a infinite, steamed, deferred, side-effect-free, query-able enumerable :)
