Returning consonants of a plane's starting and ending points

Question

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());
    }
}

Answer 1

Perhaps I'm over-thinking this but ...

I want to create a method that will return the first and the last consonant of the plane's starting point (London)

• What should be returned if the plane's starting point is one of the places listed in List of short place names, for example A or Hồ or Yu?
• What if the place name is somewhere like 東京?
• Is the letter y a consonant or a vowel?

Part of programming is to be clear about the 'functional specification' or 'requirements' for the program, and to think about 'edge cases' or 'exceptions' (like, what if the network is down, what if the bank account is empty, what if the user is already signed in or signed out, etc.).

As for your code ...

---

This is OK ...

    string starting = direction.Split('-')[0];
    string destination = direction.Split('-')[1];

... but it's my habit to avoid making more API calls than I must. So I might call it as:

string[] split = direction.Split('-');
Debug.Assert(split.Length == 2);
string starting = split[0];
string destination = split[1];

---

This is expensive ...

starting.ToUpper().Replace("A", "").Replace("E", "").Replace("I", "").Replace("O", "").Replace("U", "");

... because there are many string operations: each Replace creates a new string. It may return the wrong answer too: the ToUpper() means that you return 'N' instead of 'n' as the last consonant in "London".

---

This is obviously just a 'test' or 'example' class ...

public class Flight
{
    private string direction;

    public Flight(string direction)
    {
        this.direction = direction;
    }

    public string Consonants()
    {
        string starting = direction.Split('-')[0];

        ... etc ...
    }
}

... so I can't comment on whether this is good "object-oriented design". Another way to present this code would be as a 'static' or more stand-alone (doesn't depend on instance data) method, for example like this:

public static class Flight
{
    // Expects input string format like "London - Berlin", returns "LN-BN"
    public static string GetConsonants(string direction)
    {
        string starting = direction.Split('-')[0];

        ... etc ...
    }
}

Answer 2

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());
}

Answer 3

This code will "work", and in any imaginable sane use case it will work "fast enough". So I feel like my comments are pedantic; nevertheless:

Variable declarations

I think this is often a matter of opinion, but I prefer to use var unless absolutely necessary. Specifying type is usually redundant and distracting, whereas var conveys what the programmer actually meant: "Just make me a variable, I don't care about the type, figure out yourself which one is correct, you have type safety anyway!"

var starting = direction.Split('-')[0];
var destination = direction.Split('-')[1];

The compiler will use C#'s strict typing features to resolve the appropriate type for each variable (based on return type of string.Split in this case) and replace the var with string before generating CIL bytecode. Note that this is at compile time - it is just syntactic sugar with no bearing on program logic, unlike, say, JavaScript's var.

Calling split twice

You split direction twice. Since the algorithm isn't "split string, take first part, split string, take second part", the implementation is confusing to read. A more natural approach would be "split string, distribute two parts accordingly":

var parts = direction.Split('-');
var starting = parts[0];
var destination = parts[1];

This inflates your line count a bit, but it is more intuitive, and slightly more efficient (especially if there were many tokens, the code was deep inside a loop and/or the splitting pattern was a complicated Regex that takes a long time to run). Plus, you can inspect the outcome of the split easily when debugging in Visual Studio by mousing over parts.

Inefficient consonant search

Your replace algorithm will run over the entire string. However, you don't actually care about the characters between the first and second consonants. You could implement a more appropriate algorithm:

i=0, j=0
head_found = false, tail_found = false
while !(head_found | tail_found)
    if(!head_found)
        if(Is_consonant(s[i])) head_found = true;
        else i++
    if(!tail_found)
        if(Is_consonant(s[j])) tail_found = true;
        else j--        
return [i, j]

Your current algorithm runs in linear time (proportional to length of input strings). The solution above runs in constant time (proportional to mean of the Poisson distribution describing the number of leading/trailing vowels in city names).

For this particular case, you could probably get away with even simple recursive functions:

int Index_of_first_consonant(string s)
    if(Is_consonant(s.First())) return 0
    else return 1+Index_of_first_consonant(s.Substring(1))

int Index_of_last_consonant(string s)
    if(Is_consonant(s.Last())) return s.Length-1
    else return Index_of_last_consonant(s.Take(s.Length-1))

Inefficient lookup

For what you are doing, the typical solution is to use either Dictionary or HashSet<char> which includes both lowercase and uppercase consonants (the extra space usage is trivial in this case). You will have to predefine it:

private static HashSet<char> consonants = new HashSet<char>{ 'B', 'b', 'C', 'c', 'D', 'd' /* TODO: include all consonants */ };

Unfortunately var cannot be used for fields. If you are lazy, you can add a static constructor for your class and populate this consonants programmatically, with your inefficient vowel exclusion method (efficiency won't matter because the static constructor will run only once per program execution). But you should still use a HashSet for tasks like these, which essentially test membership of a set.

Major bugs

Your code will crash for strings which don't have enough consonants. (Strangely named cities or erroneously provided null strings) You can fix extra whitespace with myString.Trim(), but that won't handle symbols (for example, one way of transliterating Cyrillic names can generate words ending with ').

You also don't handle non-letters correctly. With your example input London - Berlin, I think the first consonant of the destination and the last consonant of the starting point will be returned as spaces.

Naming

The common convention is to use nouns for properties and fields, and verbs (often in the imperative) for methods. Also, your method Consonants doesn't actually produce the consonants, it produces a string describing first and last consonants. Therefore the name is a bit misleading, you might consider something like Print_first_last_consonants.

Return expression

My personal preference is to assign return values to an intermediate variable first.

var result = /* calculate result here */;
return result;

This way, when your program crashes and you come across this method while trudging through the stack, it will be easier for you to decide if the error propagated to this method or not.

Also, for the actual calculation, I would have used

startConsonants.First() + startConsonants.Last() + "-" + destConsonants.First() + destConsonants.Last()

since it is easier to read, unless you anticipate that you will be changing the output format drastically and/or frequently.

Answer 4

I think there is room for improvement in the following areas:

1. Duplication of logic and data (this harms maintainability and readability)
2. Mixing of different business concepts (flight endpoints, flights paths, flight direction parsing)
3. Cluttered logic is hard to read and maintain. Breaking it up into well-named variables, classes and methods helps fix this.
4. Use of noun-named method (Consonants() as opposed to Consonants property or MakeConsonants() method)

I originally wrote out step-by-step instructions to address these concerns, but it got too long, so I'm instead only including a suggested revision of your code with the above problems addressed:

public class Flight
{
    private const char endpointSeparator = '-';
    private string direction;

    public Flight(string direction)
    {
        this.direction = direction;
    }

    public string Consonants
    {
        get
        {
            return String.Join(endpointSeparator.ToString(), EndpointAbbreviations);
        }
    }

    private IEnumerable<string> EndpointAbbreviations
    {
        get
        {
            return EndpointNames.Select(Abbreviate);
        }
    }

    private string[] EndpointNames
    {
        get
        {
            return direction.Split(endpointSeparator);
        }
    }

    private static string Abbreviate(string endpointName)
    {
        var endpoint = new FlightEndpoint(endpointName);
        return endpoint.Abbreviation;
    }
}

public class FlightEndpoint
{
    private readonly string name;

    public FlightEndpoint(string name)
    {
        this.name = name;
    }

    public string Abbreviation
    {
        get
        {
            return new string(UppercaseFirstAndLastConsonants);
        }
    }

    private char[] UppercaseFirstAndLastConsonants
    {
        get
        {
            IEnumerable<char> uppercaseConsonants = UppercaseConsonants;
            return new[] { uppercaseConsonants.First(), uppercaseConsonants.Last() };
        }
    }

    private IEnumerable<char> UppercaseConsonants
    {
        get
        {
            return UppercaseName.Where(IsConsonant);
        }
    }

    private string UppercaseName
    {
        get
        {
            return name.ToUpper();
        }
    }

    private static bool IsConsonant(char uppercaseCharacter)
    {
        return !IsVowel(uppercaseCharacter);
    }

    private static bool IsVowel(char uppercaseCharacter)
    {
        return "AEIOU".Contains(uppercaseCharacter);
    }
}

Notes about the above code:

1. Methods are organised for easy reading from top to bottom.
2. Methods are kept short for better code readability

Further suggestions:

1. Create a FlightDirection or FlightPath class that consists of the FlightEndpoints and the Consonants operation to separate the operation from the direction parsing logic.
2. Extract out the code related to vowels and letter case if it will or could be used in other code.
3. Rename Consonants since it reads as if it gets all consonants. Perhaps there is a business term that is more clear, such as Abbreviation or ShortName.