Ocena kodu źródłowego

Question

Dzień dobry, chciałbym prosić o ocenę kodu.

Klasa ma dwie metody publiczne:

1. doesItHaveOnlyLettersDigitsDollarsAndUnderscores sprawdza, czy słowo spełnia wymagania algorytmu.

2. makeShorter skraca podane słowo do maksymalnej długości według poniższego algorytmu:

napisz nazwę zmiennej w postaci, w której życzyłbyś sobie ją widzieć – używaj tylko liter, cyfr oraz znaków '_' (podkreślenie) i '$' (dolar); może zabrzmi to dziwnie, ale znak '$' traktować będziemy jak literę (czyni to do dzisiaj wiele kompilatorów);
 
jeśli długość nazwy jest mniejsza bądź równa n, możesz jej użyć i nie musisz robić nic więcej
 
w przeciwnym wypadku usuwaj z nazwy, począwszy od końca, wszystkie znaki, które nie są literami i cyframi – w chwili, w której długość nazwy osiągnie n, możesz zakończyć pracę i użyć nazwy zmiennej
 
jeśli długość nazwy nadal jest większa od n, usuwaj z niej, począwszy od końca, kolejne cyfry - w chwili, w której długość nazwy osiągnie n możesz zakończyć pracę i użyć nazwy zmiennej
 
jeśli długość nazwy nadal jest większa od n, usuwaj z niej, począwszy od początku, kolejne samogłoski z wyjątkiem pierwszej (chodzi o to, by w nazwie została chociaż jedna samogłoska, o ile jakakolwiek została użyta) - w chwili, w której długość nazwy osiągnie n, możesz zakończyć pracę i użyć nazwy zmiennej
 
jeśli długość nazwy nadal jest większa od n, usuwaj z niej znaki od końca, począwszy od przedostatniego - w chwili, w której długość nazwy osiągnie n, możesz zakończyć pracę i użyć nazwy zmiennej

Do testów jednostkowych wykorzystany został CppUnitTest. Assert::AreEqual(expected, actual).

WordShortenerTest.cpp

#include "CppUnitTest.h"
#include "../WordShortener/WordShortener.h"

using namespace Microsoft::VisualStudio::CppUnitTestFramework;

namespace UnitTest {
	TEST_CLASS(WordShortenerTest)
	{
	public:

		TEST_METHOD(testForCorrectData)
		{
			WordShortener ws;
			Assert::IsTrue(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("ABCDEFGHIJKLMNOPRSTUVWXYZabcdefghijklmnoprstuvwxyz1234567890_$"));
		}

		TEST_METHOD(testForIncorrectData)
		{
			WordShortener ws;

			//Control characters
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("\a"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("\b"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("\t"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("\n"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("\v"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("\f"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("\r"));
			//Punctuation characters
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("\""));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("\'"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("\\"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("\?"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("!"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores(" "));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores(","));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("."));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores(":"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores(";"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("-"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("("));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores(")"));
			//Other characters
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("+"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("*"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("/"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("="));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores(">"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("<"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("#"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("%"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("`"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("|"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("~"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("{"));
			Assert::IsFalse(ws.doesItHaveOnlyLettersDigitsDollarsAndUnderscores("}"));
		}

		TEST_METHOD(testForWordSizeLowerThanSpecifiedMaxSize)
		{
			WordShortener ws;

			Assert::AreEqual(std::string("ABC"), ws.makeShorter("ABC", 5));
		}

		TEST_METHOD(testForWordSizeEqualToSpecifiedMaxSize)
		{
			WordShortener ws;

			Assert::AreEqual(std::string("ABC"), ws.makeShorter("ABC", 3));
		}

		TEST_METHOD(testForWordWithTooManyUnderscores)
		{
			WordShortener ws;

			Assert::AreEqual(std::string("ABC"), ws.makeShorter("___A___B___C___", 3));
			Assert::AreEqual(std::string("___A_BC"), ws.makeShorter("___A___B___C___", 7));
		}

		TEST_METHOD(testForWordWithTooManyDigits)
		{
			WordShortener ws;
			Assert::AreEqual(std::string("ABC"), ws.makeShorter("123A456B789C0", 3));
			Assert::AreEqual(std::string("123A45BC"), ws.makeShorter("123A456B789C0", 8));
		}

		TEST_METHOD(testForWordWithTooManyVowels)
		{
			WordShortener ws;

			Assert::AreEqual(std::string("ABBBBUB"), ws.makeShorter("ABEBIBOBUB", 7));
			Assert::AreEqual(std::string("abbbbub"), ws.makeShorter("abebibobub", 7));
		}

		TEST_METHOD(testForWordWithTooManyConsonants)
		{
			WordShortener ws;

			Assert::AreEqual(std::string("BCDFZ"), ws.makeShorter("BCDFGHJKLMNPQRSTVWXYZ", 5));
			Assert::AreEqual(std::string("bcdfz"), ws.makeShorter("bcdfghjklmnpqrstvwxyz", 5));
		}

		TEST_METHOD(testForWordWithTooManyDollars)
		{
			WordShortener ws;

			Assert::AreEqual(std::string("$B$CF"), ws.makeShorter("$B$C$D$F", 5));
			Assert::AreEqual(std::string("B$C$$"), ws.makeShorter("B$C$D$F$", 5));
		}

	};
}

WordShortener.h

#pragma once

#include <string>

class WordShortener {
public:
	bool doesItHaveOnlyLettersDigitsDollarsAndUnderscores(const std::string& word) const;

	std::string makeShorter(const std::string& word, const std::size_t& maxSize) const;

private:
	void deleteUnderscores(std::string& source, const size_t& maxSize) const;

	void deleteDigits(std::string& source, const size_t& maxSize) const;

	void deleteVowelsWithoutFirst(std::string& shortenedWord, const size_t& maxSize) const;

	void deleteCharactersWithoutLast(std::string& source, const size_t& maxSize) const;

	bool isDollar(char character) const { return character == '$'; }

	bool isUnderscore(char character) const { return character == '_'; }

	bool isVowel(char character) const;
};

WordShortener.cpp

#include "WordShortener.h"
#include <cctype>

bool WordShortener::doesItHaveOnlyLettersDigitsDollarsAndUnderscores(const std::string& word) const
{
	for (char character : word)
		if (!isalnum(character) && !isDollar(character) && !isUnderscore(character))
			return false;
	return true;
}

std::string WordShortener::makeShorter(const std::string& word, const std::size_t& maxSize) const
{
	if (word.size() <= maxSize)
		return word;

	std::string shortenedWord(word);

	deleteUnderscores(shortenedWord, maxSize);
	if (shortenedWord.size() <= maxSize)
		return shortenedWord;

	deleteDigits(shortenedWord, maxSize);
	if (shortenedWord.size() <= maxSize)
		return shortenedWord;

	deleteVowelsWithoutFirst(shortenedWord, maxSize);
	if (shortenedWord.size() <= maxSize)
		return shortenedWord;

	deleteCharactersWithoutLast(shortenedWord, maxSize);
	return shortenedWord;
}

void WordShortener::deleteUnderscores(std::string& source, const size_t& maxSize) const
{
	for (std::size_t i = source.size() - 1; i > 0 && source.size() > maxSize; i--)
		if (isUnderscore(source[i]))
			source.erase(i, 1);
	if (source.size() > maxSize && isUnderscore(source.front()))
		source.erase(0, 1);
}

void WordShortener::deleteDigits(std::string& source, const size_t& maxSize) const
{
	for (std::size_t i = source.size() - 1; i > 0 && source.size() > maxSize; i--)
		if (isdigit(source[i]))
			source.erase(i, 1);
	if (source.size() > maxSize && isdigit(source.front()))
		source.erase(0, 1);
}

void WordShortener::deleteVowelsWithoutFirst(std::string& shortenedWord, const size_t& maxSize) const
{
	bool firstVowel = true;
	for (std::size_t i = 0; i < shortenedWord.size() && shortenedWord.size() > maxSize; i++) {
		if (isVowel(shortenedWord[i]) && firstVowel)
			firstVowel = false;
		else if (isVowel(shortenedWord[i]))
			shortenedWord.erase(i, 1);
	}
}

void WordShortener::deleteCharactersWithoutLast(std::string& source, const size_t& maxSize) const
{
	for (std::size_t i = source.size() - 2; i > 0 && source.size() > maxSize; i--)
		source.erase(i, 1);
	if (source.size() > maxSize)
		source.erase(0, 1);
}

bool WordShortener::isVowel(char character) const
{
	char lower = tolower(character);
	switch (lower) {
	case 'a':
	case 'e':
	case 'i':
	case 'o':
	case 'u':
		return true;
	default:
		return false;
	}
}

 

Answer 1

1. Te 30 linijek asertów mozna skrócić do pęlti iterującej po stringu skladajacym sie ze znakow ktore chcesz sprawdzic

2. void deleteUnderscores(std::string& source, const size_t& maxSize) const;
   

   Programowanie proceduralne. Przesyłasz string miedzy 4 metodami zamiast zrobic prywatne pole.

3. To raczej osobista preferencja, ale switch case w isVowel jest brzydki. Może w ten sposob:

   char lower = tolower(character);
   return lower == 'a' || lower =='e' || lower =='i' || lower=='o' || lower=='u';
   //albo:
   return std::string("aeiouy").find(toLower(character)) != std::string::npos;
   

4. source.erase(i, 1);

   Poczytaj jak działa string pod spodem. Wszelkie erase lub inserty w srodku lub na początku wiążą się z przepisaniem całego stringa w nowe miejsce. Dlatego jest coś takiego jak erase-remove idiom. Jeśli nie chcesz idiomem to po prostu twórz kopie i przepisuj. Będzie to lepsze niż kilkakrotny erase w srodku stringa.

5. isDolar, isUnderscore to troszke przesada. == '_' jest rownie czytelne.

 

Comments

Punkt 1 nie będzie czasem łamał DAMP?

---

Bardzo możliwe, jak to fajnie nauczyć się czegoś nowego. Dzieki :P

Answer 2

1. Korzystaj z gotowych algorytmów. Zamiast wielokrotnego używania erase lepiej użyć std::remove/std::remove_if i dopiero wtedy zwolnić niepotrzebny nadmiar:

   void WordShortener::deleteDigits(std::string& source, const size_t& maxSize) const
   {
   	for (std::size_t i = source.size() - 1; i > 0 && source.size() > maxSize; i--)
   		if (isdigit(source[i]))
   			source.erase(i, 1);
   	if (source.size() > maxSize && isdigit(source.front()))
   		source.erase(0, 1);
   
   	
   }
   
   void WordShortener::deleteVowelsWithoutFirst(std::string& shortenedWord, const size_t& maxSize) const
   {
   	bool firstVowel = true;
   	for (std::size_t i = 0; i < shortenedWord.size() && shortenedWord.size() > maxSize; i++) {
   		if (isVowel(shortenedWord[i]) && firstVowel)
   			firstVowel = false;
   		else if (isVowel(shortenedWord[i]))
   			shortenedWord.erase(i, 1);
   	}
   }

   source.erase(std::remove_if(source.begin(), source.end(), isUnderscore), source.end()); // <algorithm>
   source.erase(std::remove_if(source.begin(), source.end(), ::isdigit), source.end()); // <algorithm>

   Podobnie z deleteVowelsWithoutFirst, deleteCharactersWithoutLast. Wystaczy odpowiednio wyszukać pierwszą bądź ostatnią za pomocą std::string::find_first_of
2. WordShortener::isVowel - wincyj DRY:

   bool WordShortener::isVowel(char character) const
   {
   	static const std::unordered_set<char> vowels = { 'a', 'e', 'i', 'o', 'u' }; // zamiast std::unordered_set np std::array i wtedy std::find
   	return vowels.find(tolower(character)) != vowels.end();
   }
   

3. W funkcji doesItHaveOnlyLettersDigitsDollarsAndUnderscores możesz użyć std::all_of
4. Nazwa doesItHaveOnlyLettersDigitsDollarsAndUnderscores jest bardzo długa. Może da się ją zmienić na krótszą? ;)

Comments

WordShortener::isVowel - wincyj DRY:

Chyba KISS?

bool WordShortener::isVowel(char character) const
{
	return strchr("aeiou", tolower(character)) != nullptr;
}