exam-check-1

(коментарите тип "Program.cs:30" съм си ги сложил предварително и се отнасят за оригиналния сорс който ползвам за сравнение)

Code Refactoring

    Name well the bad identifiers in the code

      Bad identifiers: ConsoleApplication1, Problem_2, Class2, code, data, REP, REPNew, input (should be output), i, j, k, s, strings, str0, str1, conv, flag, Class1, Class2, sb, name2, Strings, old, obj, obj2, num, nums, oldent, newent, list, sorted, dict, multidict, found, ...

        Identifiers are bad

        --> Identifiers need more improvement
            REP, REPNew, etc. CommandValidator е по-скоро CommandExecutor. OutputResultManager е по-скоро някакъв utility class, макар че с този си по-скоро не трябва да съществува

        Indentifiers are well named

    Format the code well

        Code formatting is bad

        Code formatting is average

        --> Code formatting is good

        на някои места има доста дълги редове които би било добре да се разделят:
          if (phoneNumberDigits.Length >= 2 && phoneNumberDigits[0] == '0' && phoneNumberDigits[1] == '0')

        Code formatting is excellent

    Remove unused using statements: System.Data.* and System.Net.*

        Unused usings are removed

        Unused usings are partially removed

        --> Unused usings are NOT removed

        из доста от файловете имаш ненужни using-и

    Remove meaningless / wrong comments

        --> left // this works!

        removed // this works!

    Remove meaningless / wrong comments

        left // Error reading from console

        --> removed // Error reading from console

    Remove meaningless / wrong comments

        left // first command, // second command, // third command

        partially left // first command, // second command, // third command

        --> removed // first command, // second command, // third command

    Remove the unneeded namespaces

      (at least one of them)

        No

        --> Yes

    Fix incorrect error handling

        // Program.cs:30
        The code “Console.WriteLine("error!"); Environment.Exit(0);” is left untouched

        The code “Console.WriteLine("error!"); Environment.Exit(0);” is partially fixed

        --> Appropriate exception is thrown instead of the code “Console.WriteLine("error!"); Environment.Exit(0);”

    Fix incorrect error handling

        // Program.cs:37
        The incorrect recursive call “Main();” is left untouched

        Partially fixed the incorrect recursive call “Main();”

        --> Changed the incorrect recursive call “Main();” with throwing an exception

    Fix incorrect error handling

        // Program.cs:60
        The code “throw new StackOverflowException();” is left untouched

        Partially fixed the code “throw new StackOverflowException();”

        --> A correct exception with correct message is thrown instead of “throw new StackOverflowException();”

    Fix incorrect error handling

        // REPNew.cs: 43
        --> The line “Console.WriteLine("Duplicated name in the phonebook found: " + name);” is left untouched

        Partially fixed the “Console.WriteLine("Duplicated name in the phonebook found: " + name);”

        The line “Console.WriteLine("Duplicated name in the phonebook found: " + name);” changed to throw an exception

    Fix incorrect error handling

        // REP.cs:62
        --> The line “Console.WriteLine("Invalid start index or count.");” is left untouched

        Partially fixed the “Console.WriteLine("Invalid start index or count.");”

        The line “Console.WriteLine("Invalid start index or count.");” changed to throw an exception

    Long methods are split into small, simple and well named sub-methods

        // Program.cs:16
        Method Main() is left too long

        --> Method Main() is partially refactored

        за съжаление не е много добре рефакториран (парсването и switch-а са заровени в един и същи метод, проверката за null е безсмислена), имаш и грешки (при грешен брой аргументи хвърляш "invalid command name")

        Method Main() is split into several simpler methods

    Removed the bad cohesion in the method Cmd() or its renamed form

        // Program.cs:66
        A parameter says what the method should do instead of having 3 separate methods

        --> Тhe method Cmd(…) still has a parameter that indicates what operation to execute

        Тhe method Cmd(…) is partially refactored and improved

        Тhe method Cmd(…) is split into 3 methods with better cohesion: for adding a phone, for changing a phone, for listing phones

    Simplify complex expressions

      // Program.cs:89
      look around "numbers changed"

        The expression is left untouched: Print("" + data.ChangePhone(conv(strings[0]), conv(strings[1])) + " numbers changed");

        The expression is partially simplified: Print("" + data.ChangePhone(conv(strings[0]), conv(strings[1])) + " numbers changed");

        --> Split the complex expression into few meaningful sub-expressions: Print("" + data.ChangePhone(conv(strings[0]), conv(strings[1])) + " numbers changed");

    Simplify complex expressions

      // Program.cs:94
      look around int.Parse(...)

        --> The expression is left untouched: IEnumerable<Class1> entries = data.ListEntries(int.Parse(strings[0]), int.Parse(strings[1]));

        The expression is partially simplified: IEnumerable<Class1> entries = data.ListEntries(int.Parse(strings[0]), int.Parse(strings[1]));

        Split the complex expression into few meaningful sub-expressions: IEnumerable<Class1> entries = data.ListEntries(int.Parse(strings[0]), int.Parse(strings[1]));

    Make public the IPhonebookRepository interface

        The interface IPhonebookRepository is NOT public

        --> The interface IPhonebookRepository is public

    Make public the major interfaces and classes

        The classes implementing the IPhonebookRepository interface (REP and REPNew) are NOT public

        --> The classes implementing the IPhonebookRepository interface (REP and REPNew) are public

    Make public the major interfaces and classes

        The "phonebook entry" class is NOT public

        --> The "phonebook entry" class is public

    Make public the main program class

      (this will be needed for the unit tests)

        The main program class is NOT public

        --> The main program class is public

    Move each public class / interface into separate C# file

        All classes and interfaces stay in the same file

        Some classes / interfaces are in separate files, some are in the same file

        --> All classes and interfaces stay in separate files

    Refactor the public fields

      // Entry.cs:25
      public SortedSet<string> Strings;

        --> The field is left untouched: public SortedSet<string> Strings;

        преименувано но не променено

        The field is partially fixed: public SortedSet<string> Strings;

        The field is made private or converted to a read-only propety: public SortedSet<string> Strings;

    Refactor the public fields

      // REPNew.cs:7
      public List<Class1> entries = new List<Class1>();

        --> The field is left untouched: public List<Class1> entries = new List<Class1>();

        The field is partially fixed: public List<Class1> entries = new List<Class1>();

        The field is made private or converted to a read-only propety: public List<Class1> entries = new List<Class1>();

        The field is missing because its entire class is removed.

Bug Fixing

    // Program.cs:80
    Change "Phone entry created." to "Phone entry created"

      (with no dot at the end)

        --> Not fixed

        Fixed

    // Program:cs - Cmd
    The code for processing the commands “AddPhone” and “List” is interchanged

        Not fixed

        --> Fixed

    // Program:cs - 50
    The command “ChangeРhone” is wrongly spelled

      (with Cyrillic „Р“ instead of Latin letter “P”)

        --> Not fixed

        Fixed

Unit Testing

    Method IPhonebookRepository.AddPhone(): single phonebook entry

      Add а single phonebook entry and assert it is correctly entered in the repository as new entry

        --> No

        Partially

        Yes

    Method IPhonebookRepository.AddPhone(): duplicated entry

      Add duplicated entry (add several times the same name + phones) and assert the entry is correctly entered in the repository only once. The first call to AddPhone() should return true, while the next calls should return false.

        --> No

        Partially

        Yes

    Method IPhonebookRepository.AddPhone(): different casing

      Add multiple phones for the same name passed with different casing (e.g. “AddPhone(Nakov, +359111)”, then “AddPhone(naKOV, +359222)”) and assert only one phonebook entry is created. The first call to AddPhone() should return true, while the next calls should return false.

        --> No

        Partially

        Yes

    Method IPhonebookRepository.AddPhone(): add with merge

      Test add with merge: add several sets of phones for the same name and assert that in the repository the set of phones are intersected (all unique phones are kept while all duplicates are removed).

        --> No

        Partially

        Yes

    Method IPhonebookRepository.ListEntries(…): single entry

      Single entry with single phone / multiple phones: test adding a single entry with a single phone and / or multiple phones and list it, then check for correctness

        --> No

        Partially

        Yes

    Method IPhonebookRepository.ListEntries(…): sorting the phonebook entries

      Test sorting the phonebook entries: add several entries, list them and assert their sorting order is correct

        --> No

        Partially

        Yes

    Method IPhonebookRepository.ListEntries(…): sorting the phones within an entry

      Test sorting the phones within an entry: add an entry with several phones, list them and assert the phones are sorted correctly

        --> No

        Partially

        Yes

    Method IPhonebookRepository.ListEntries(…): merge + list

      Merge + list: add few phonebook entries with the same name to cause their phones to be merged. Then list the added entries and assert the merge was correct

        --> No

        Partially

        Yes

    Method IPhonebookRepository.ListEntries(…): paging

      Paging: add n entries, then list less than n entries and assert the extracted page is correct. E.g. if we have 5 entries, try List(1, 3) and assert the result is correct

        --> No

        Almost no

        Partially

        Not fully

        Yes

    Method IPhonebookRepository.ListEntries(…): invalid range - negative start

      List invalid range (negative start): try to list an invalid range (negative start) and assert an exception is thrown

        --> No

        Yes

    Method IPhonebookRepository.ListEntries(…): invalid range - too big start

      List invalid range (invalid start): try to list an invalid range (too big start) and assert an exception is thrown

        --> No

        Yes

    Method IPhonebookRepository.ListEntries(…): invalid range - too big count

      List invalid range (invalid count): try to list an invalid range (too big count) and assert an exception is thrown

        --> No

        Yes

    Method IPhonebookRepository.ChangePhone(…): change existing phone

      Change existing phone: add a phonebook entry, then change its phone, assert 1 phone number is affected and list the phone and check it is correct

        --> No

        Partially

        Yes

    Method IPhonebookRepository.ChangePhone(…): change non-existing phone

      Change non-existing phone: change a non-existing phone and assert the number of affected phones is 0 and no changes were made in the phonebook repository

        --> No

        Partially

        Yes

    Method IPhonebookRepository.ChangePhone(…): change shared phone

      Change shared phone: add several phonebook entries that have a common (shared) phone number; change the common phone number; assert the number of affected phones is correct; list the phones and assert they are correct

        --> No

        Almost no

        Partially

        Not fully

        Yes

    Method IPhonebookRepository.ChangePhone(…): marge with duplicates

      Change phone with merge and duplicates: add few phonebook entries; change a phone number with another phone number already in the phonebook to cause duplicated phones (obtained after the change) to be merged; list the phonebook and assert the phones are correctly merged

        --> No

        Partially

        Not fully

        Yes

    Check the code coverage for the class implementing IPhonebookRepository

        --> 0% ... 24%

        25% ... 54%

        55% ... 84%

        85% ... 100%

Code Documentation

    Document all public methods and properties defined in the IPhonebookRepository interface using XML documentation

      The documentation should be in the IPhonebookRepository interface

        No documentation

        Some documentation exists, but far away from finished

        Some interface methods documented partially

        Most interface methods documented partially

        All interface methods documented partially (or documentation is in the class instead in the interface)

        --> All interface methods documented well

Performance Bottlenecks

    // Program.cs:12
    IPhonebookRepository implementation (look around "static IPhonebookRepository")

      Boottleneck description: the program uses the slower IPhonebookRepository implementation (REPNew instead of REP). The implementation based on list (REPNew) is many times slower than the implementation based on ordered sets and dictionaries (REP)

        --> Bottleneck NOT found

        Bottleneck found (described somehow in the code, but not fixed)

        Bottleneck found and fixed (the faster IPhonebookRepository implementation is used)

    // Program.cs:104
    Phone convertion to canonical form (look around "IsDigit()")

      The performance bottleneck is in the method “conv(…)” that converts a phone number into canonical form. It has unneeded for-loop that does nothing but repeating the phonebook conversion logic many times. In the loop body the conversion is made several times with no good reason.

        --> Bottleneck NOT found

        Bottleneck found (described somehow in the code, but not fixed)

        Bottleneck found and fixed (remove the unneeded for-loop and repeated conversion logic)

    ToList() invocation in IPhonebookRepository.ChangePhone()

      // REP.cs:48
      The performance bottleneck is in the method ChangePhone() of IPhonebookRepository"s fast implementation. It performs ToList() for all matched phonebook entries which is slow operation. No fix is available.

        --> Bottleneck NOT found

        Bottleneck found, but not well described

        Bottleneck found and is correctly described