Code Quality: Modules and Functions
1)
“Does the class(/module) minimize accessibility to its members?”
Minimizing accessibility to module data is good advice, because it encourages Encapsulation. We only want to expose what’s necessary for the user, and let them interact with getters and setters. It also improves abstraction, as it allows you to ignore the implementation details.
2)
“Does the routine have strong, functional cohesion — doing one and only one thing and doing it well?”
This is better advice than “The routine should do what needs to be done”, because the first implies the other. An ideal routine should consist of a lot of routines doing one thing well. Additionally, functions should be building blocks, not specifically tailored.
3)
Description
In the listing under is a module interface (a C header file) from an old project deliverable. The module’s name is “cost”. Criticize (concisely, with bullet points) the design.
Change name of global variables to signify that they are in fact global
g_- All caps
Gather global variables (top of file)
- downCost, upCost, designatedElevator
It is not obvious how one should use every function. Overlapping functionality between some functions like downCost andlowestCostDirection. Bad abstraction.
4)
Description
This is another module interface. This time with the name of
jobqueue. Criticize it; feel free to compare it to the previous one.
- Include time in the header file makes it hard to keep track when looking in .c file
- Unnecessary comments for most routines
- Good encapsulation with get and set functions
- Not minimal, too many routines for such a simple thing.
5)
Description
This listing (under) is the function “delete_flag” from the module in the previous task. Criticize (shortly and in bullet points).
- Bad encapsulation, since this function uses
any_flags_{below/above}, and does not need to be declared in the header in the last task. - Magic numbers misused. It might be obvious that the bottom floor is 0, but not necessarly that 3 is the top floor.
- What is flag?
- It does not simply delete flags, it also meddles with the lamps. This is against the point “Do one thing, and do it well”
- Confusing if sentences
6)
Description
Look at the following c header file (…as a module interface…): (Picture) List shortly what you see as its two most important weaknesses.
- This module does way to much on its own. It defines orders and structs, it works directly on the network, hardware setters, redistributed orders etc.
- Not minimal at all, a lot of unnecessary function
7)
Description
Here are the four ’flag_’ functions from the sch_orders module: (PICTURE) Suggest shortly the main improvement potential(s). Explain why.
- Encapsulation not ideal (lack of get+set, only sometimes). No way of changing the interface later as the variable itself is directly changed.
- The last 4 lines of code are identical in all the flag functions. This could be gathered into a single function.
- All functions are almost identical
- What is
ORDER_DIRECTION_CAR?
8)
Description
What do you think of this variable?
int floor = -1;It should be set to the floor number when the lift is on a floor and to -1 when the elevator is between floors. Could you improve its name? Suggest another improvement?
The name should be changed to specify that this is not the nearest floor, or the last floor visited. For example sensorFloor could be an alternative.