imgmaterial / mnxb11_hw6 Goto Github PK

Hi,

thanks for your homework submission. Great work overall! The following is feedback on the code you submitted that I hope can be useful for you in the future, it isn't something you need to correct for the assignment. I will be focusing on the contents of the main.cxx file but similar things apply for the other one

• MNXB11_HW6/main.cxx

  Line 8 in 9383bf8

  char firstChar{*(_inputString.data())}; //pointer to a C-style string return the first character of the string

  To get the first element of a std::string, just use the [] operator

• MNXB11_HW6/main.cxx

  Line 9 in 9383bf8

  return(int)((_sum ^ firstChar * 3) << (exeLen & 0x1f));

  Don't use C-style casts. If you really have to (and you don't here) use a corresponding C++ cast instead. C-style casts are much too flexible and can lead to really surprising behavior and some nasty bugs without even giving a warning. See https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es48-avoid-casts and https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es49-if-you-must-use-a-cast-use-a-named-cast for details

• MNXB11_HW6/main.cxx

  Line 12 in 9383bf8

  bool ValidateInput(bool _correctAmountOfArguments, int _expectedKey){

  Don't decorate the names of function parameters. Function declarations (including the parameters) are what people will read to try and understand what your code is doing. You don't want to make them harder to read than what is necessary. People will often mark private member variables of objects with a _ prefix or suffix, but those are implementation details that a reader shouldn't have to look at.

• MNXB11_HW6/main.cxx

  Lines 27 to 29 in 9383bf8

  	for (int ptr = 0; ptr < (int)_inputString.size();ptr++){
  	sum += _inputString[ptr];
  	}

  This is a loop over all the elements in a container, use a range-based for loop. Also, ptr is a misleading name for an index.

• MNXB11_HW6/main.cxx

  Lines 48 to 52 in 9383bf8

  	bool correctAmountOfArguments = (argCount == 3);
  	int expectedKey = std::atoi(correctAmountOfArguments ? (argValues[2]) : "0"); //if the value of provided arguments is correct then try to convert it from string to integer, else convert "0" to integer. Avoids segfault if the arguments were not provided.
  	if (!ValidateInput(correctAmountOfArguments, expectedKey)){//print output form ValidateInput and return early if false.
  	return 0;
  	}

  This is a very roundabout and complicated way to write this. I like that you are checking the validity of the second argument, but you should really never have the ability to reach the std::atoi call without argv[2] being valid (i.e. argc >= 3)

Cheers,
Einar