BartoszWilkowski88 / OOPAssignment1

Assignment 1 Code
0 stars 0 forks source link

Code review (Cabe Towers 25708017) #5

Open Lynx-At0Mic opened 2 years ago

Lynx-At0Mic commented 2 years ago

Code review (Cabe Towers 25708017)

In this code review, I will go over each file in turn and provide constructive feedback in the form of going over all the things done well, addressing any issues, and suggesting improvements.

Analyse.cs

Good points

Issues

Improvements

Sentence matching could be improved by using regex expressions to match the start and end of sentences. This would fix any errors in edge cases where differing punctuation is used.

Some variables are unclear. For example, flstp and frqc. It's convention and best practice to avoid such abbreviations as it can make the code hard to understand for someone who is unfamiliar with it. I suggest renaming the variables to fullstop and frequency respectively.

The implementation is solid, although inefficient. Consider the following example from your code.

string vowels = "aeiouAEIOU";
...
foreach (char c in input)
{
    ...
    foreach (char vow in vowels)
        {
            if (vow == c)
            {
                values[1]++;
            }
    }
    ...
}

This implementation is well thought out and shows a solid logical process. However, this could be improved since looping over each character is quite inefficient (especially if it's done multiple times for each character in the input text). An improved solution could be to use a char array instead of a string for the vowels and other variables. Below is an example

char[] vowels = "aeiouAEIOU".ToArray();
...
foreach (char c in input)
{
    ...
    if (vowels.Contains(c))
    {
        values[1]++;
    }
    ...
}
...

This could be improved even further using regex to avoid looping in the first place and compress the tally process down into just 1 line!

values[1] = Regex.Matches(input, @"[aeiou]").Count();

If you are not familiar with regex I would highly suggest following a tutorial as it's a powerful tool that makes operations like this very easy and quick to implement

Input.cs

Good points

Issues

Improvements

Although you cover file not found errors with a guard clause in Program.cs I suggest implementing error handling for IOException, UnauthorizedAccessException, and FileNotFoundException using try-catch statements. This will help secure 2:2 and First standard checklists and ensure the program does not crash while being assessed. An example is shown below.

public string fileTextInput(string fileName)
{
    try
    {
        text = File.ReadAllText(fileName);
    }
    catch (ExceptionGoesHere e)
    {
        // Handle exception here
    }
    ...
    return text;
}

Program.cs

Good points

Issues

Improvements

Nothing major, non-essential things general code tidiness and spacing are nice but in no way required.

LongWords.cs

Good points

Issues

Improvements

Ensure variable names make sense and follow a convention like camel case. For example, I suggest renaming ind' toindexand ensuring variables likelngwrdfileare not abbreviated and follow a convention like camel case, eg:longWordFile`

Report.cs

Good points

Issues

Improvements

Overall

There are a few minor issues and improvements which can be made but nothing major. One recurring issue was variable naming conventions. Although this is not stated as required by the brief, it is best practice to use variable names that make sense, follow a convention like camel case, and avoid abbreviations where possible. Doing this helps make the code more readable and will provide further evidence towards higher grades.

In general, a very good implementation. All features discussed in the brief are implemented and results are correct apart from edge cases. During testing, user input was handled correctly and did not error out when invalid or null inputs were given. With some minor improvements this is easily a First standard assignment, well done.

BartoszWilkowski88 commented 2 years ago

I was so focused on all the other parts that I forgot to use the manualTextInput(). Glad you noticed it. I also appreciate the tutorial you attached, for sure going to give it a read about Regex. For sure will be adding some exception handling as well. Thank you for your review. Much appreciated.