HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Returning consonants of a plane's starting and ending points

Submitted by: @import:stackexchange-codereview··
0
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:

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 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.