patterncsharpModerate
Returning consonants of a plane's starting and ending points
Viewed 0 times
startingpointsplanereturningconsonantsandending
Problem
I am just starting to program in C#, so I am a beginner. I've been practicing some coding and I would like your opinion on something.
I have a flying direction for a plane, e.g. ''London - Berlin'' (direction). I want to create a method that will return the first and the last consonant of the plane's starting point (London), and the first and the last consonant of the plane's destination (Berlin).
I have written this, so I would like to know if it's ok, or if you have some suggestions:
I have a flying direction for a plane, e.g. ''London - Berlin'' (direction). I want to create a method that will return the first and the last consonant of the plane's starting point (London), and the first and the last consonant of the plane's destination (Berlin).
I have written this, so I would like to know if it's ok, or if you have some suggestions:
public class Flight
{
private string direction;
public Flight(string direction)
{
this.direction = direction;
}
public string Consonants()
{
string starting = direction.Split('-')[0];
string destination = direction.Split('-')[1];
string startConsonants = starting.ToUpper().Replace("A", "").Replace("E", "").Replace("I", "").Replace("O", "").Replace("U", "");
string destConsonants = destination.ToUpper().Replace("A", "").Replace("E", "").Replace("I", "").Replace("O", "").Replace("U", "");
return string.Format("{0}{1}-{2}{3}", startConsonants[0].ToString(), startConsonants[startConsonants.Length-1].ToString(), destConsonants[0].ToString(), destConsonants[destConsonants.Length-1].ToString());
}
}Solution
Your code is working but repeating all those
So first, I would start to think about reducing repeats in line:
You can do, for example, use LINQ:
And then simply, instead of operating on arrays
This code is more clear than yours because it has clear intention: select only not vowels and then take first and last of them. No repeating code, no indices calculation etc.
Then, to not repeat line (1) two times, I would create a method for it:
and then:
Eventually, to go along with .NET/C# ideology, I would create an extension methods for
and then:
Replace seems to be unnecessary. Firstly, you repeat them five times for single word and then once again two times because of processing two words. Code becomes long and unclear. Always, when we repeat code, it is good time to think about improvements and reducing those repeats.So first, I would start to think about reducing repeats in line:
string startConsonants = starting.ToUpper().Replace("A", "").Replace("E", "").Replace("I", "").Replace("O", "").Replace("U", "");You can do, for example, use LINQ:
string vowels = "aeiou";
var consonants = starting.Where(c => !vowels.Contains(char.ToLowerInvariant(c))); // (1)And then simply, instead of operating on arrays
startConsonants[0].ToString() and startConsonants[startConsonants.Length-1].ToString(), you can write:char first = consonants.First();
char last = consonants.Last();This code is more clear than yours because it has clear intention: select only not vowels and then take first and last of them. No repeating code, no indices calculation etc.
Then, to not repeat line (1) two times, I would create a method for it:
public string GetConsonants(string input)
{
string vowels = "aeiou";
var consonats = input.Where(c => !vowels.Contains(char.ToLowerInvariant(c)));
return new string(consonats.ToArray());
}and then:
public string Consonants()
{
string starting = direction.Split('-')[0];
string destination = direction.Split('-')[1];
string startConsonants = GetConsonants(starting);
string destConsonants = GetConsonants(destination);
return string.Format("{0}{1}-{2}{3}", startConsonants.First(), startConsonants.Last(), destConsonants.First(), destConsonants.Last());
}Eventually, to go along with .NET/C# ideology, I would create an extension methods for
Char and String to make everything clear:public static class CharExtensions
{
private static string Vowels = "aeiou";
public static bool IsVowel(this Char chr)
{
return Vowels.Contains(char.ToLowerInvariant(chr));
}
public static bool IsConsonant(this Char chr)
{
return !IsVowel(chr);
}
}
public static class StringExtensions
{
public static string OnlyConsonants(this String str)
{
return new string(str.Where(c => c.IsConsonant()).ToArray());
}
}and then:
public string Consonants()
{
string starting = direction.Split('-')[0];
string destination = direction.Split('-')[1];
string startConsonants = starting.OnlyConsonants();
string destConsonants = destination.OnlyConsonants();
return string.Format("{0}{1}-{2}{3}", startConsonants.First(), startConsonants.Last(), destConsonants.First(), destConsonants.Last());
}Code Snippets
string startConsonants = starting.ToUpper().Replace("A", "").Replace("E", "").Replace("I", "").Replace("O", "").Replace("U", "");string vowels = "aeiou";
var consonants = starting.Where(c => !vowels.Contains(char.ToLowerInvariant(c))); // (1)char first = consonants.First();
char last = consonants.Last();public string GetConsonants(string input)
{
string vowels = "aeiou";
var consonats = input.Where(c => !vowels.Contains(char.ToLowerInvariant(c)));
return new string(consonats.ToArray());
}public string Consonants()
{
string starting = direction.Split('-')[0];
string destination = direction.Split('-')[1];
string startConsonants = GetConsonants(starting);
string destConsonants = GetConsonants(destination);
return string.Format("{0}{1}-{2}{3}", startConsonants.First(), startConsonants.Last(), destConsonants.First(), destConsonants.Last());
}Context
StackExchange Code Review Q#39611, answer score: 10
Revisions (0)
No revisions yet.