patterncppMinor
Print pair representing objects from sequence of nonnegative integer pairs
Viewed 0 times
pairsnonnegativeobjectssequenceprintfromintegerpairrepresenting
Problem
There are some things I am not sure of from a professional standpoint about the "correct" way to write C++ code. I have been looking through source code produced by various opensource projects, as well as other code posted here and on Stack Overflow.
So let's just leave it at this. Let's say I am interviewing for company A over the phone. Not white board yet. They ask me to write the code below and turn it in a hour. This is a hypothetical situation, however it parallels how most "big" companies are interviewing nowadays. Would this code "get me the job"? If not, what would you change, or what can you coach me with so I can get a job programming?
```
#include
#include
#include
#include
/*
* This program reads a sequence of pairs of nonnegative integers
* less than N from standard input (interpreting the pair p q to
* mean "connect object p to object q") and prints out pair
* representing objects that are not yet connected. It maintains an
* array id that has an entry for each object, with the property
* that id[p] and id[q] are equal if and only if p and q are
* connected.
*/
static const int N = 10;
int main( int argc, char *argv[ ] )
{
/*
* Ease the type of long types
*/
typedef std::ostream_iterator output_data;
typedef typename std::vector::iterator v_it;
/*
* Generate Test Data
*/
std::pair pairs[12] = {
std::pair( 3,4 ),
std::pair( 4,9 ),
std::pair( 8,0 ),
std::pair( 2,3 ),
std::pair( 5,6 ),
std::pair( 2,9 ),
std::pair( 5,9 ),
std::pair( 7,3 ),
std::pair( 4,8 ),
std::pair( 5,6 ),
std::pair( 0,2 ),
So let's just leave it at this. Let's say I am interviewing for company A over the phone. Not white board yet. They ask me to write the code below and turn it in a hour. This is a hypothetical situation, however it parallels how most "big" companies are interviewing nowadays. Would this code "get me the job"? If not, what would you change, or what can you coach me with so I can get a job programming?
```
#include
#include
#include
#include
/*
* This program reads a sequence of pairs of nonnegative integers
* less than N from standard input (interpreting the pair p q to
* mean "connect object p to object q") and prints out pair
* representing objects that are not yet connected. It maintains an
* array id that has an entry for each object, with the property
* that id[p] and id[q] are equal if and only if p and q are
* connected.
*/
static const int N = 10;
int main( int argc, char *argv[ ] )
{
/*
* Ease the type of long types
*/
typedef std::ostream_iterator output_data;
typedef typename std::vector::iterator v_it;
/*
* Generate Test Data
*/
std::pair pairs[12] = {
std::pair( 3,4 ),
std::pair( 4,9 ),
std::pair( 8,0 ),
std::pair( 2,3 ),
std::pair( 5,6 ),
std::pair( 2,9 ),
std::pair( 5,9 ),
std::pair( 7,3 ),
std::pair( 4,8 ),
std::pair( 5,6 ),
std::pair( 0,2 ),
Solution
Looks good:
Here typename is not required:
So this:
Here I would not specify the size:
This is because you have to manually validate that the element count matches the size (this can cause subtle errors). So rather just let the compiler work it out:
Rather then specify the long winded and fully qualified type for the std::pair let the compiler deduce the type for you use std::make_pair.
I would not use an empty main branch in an if statement.
The one thing I would note is; that even though you may be using the standard library well, you are not writing encapsulated code.
I would have written the code so that the test could have been applied to the code in the same way that real data could be applied to the code. This mean you have to define an interface to your algorithm. Think about what the inputs are (how real data and test data can be provided).
Here you just have a loop inside main(). As a result you are not really showcasing your ability to write good encapsulated code.
Here typename is not required:
typedef typename std::vector::iterator v_it;So this:
typedef std::vector::iterator v_it;Here I would not specify the size:
std::pair pairs[12] = {This is because you have to manually validate that the element count matches the size (this can cause subtle errors). So rather just let the compiler work it out:
std::pair pairs[] = {Rather then specify the long winded and fully qualified type for the std::pair let the compiler deduce the type for you use std::make_pair.
std::pair pairs[] = {
std::make_pair( 3,4 ),
std::make_pair( 4,9 ),I would not use an empty main branch in an if statement.
if( t == *(start+q) )
{
}The one thing I would note is; that even though you may be using the standard library well, you are not writing encapsulated code.
I would have written the code so that the test could have been applied to the code in the same way that real data could be applied to the code. This mean you have to define an interface to your algorithm. Think about what the inputs are (how real data and test data can be provided).
Here you just have a loop inside main(). As a result you are not really showcasing your ability to write good encapsulated code.
Code Snippets
typedef typename std::vector<int>::iterator v_it;typedef std::vector<int>::iterator v_it;std::pair<int,int> pairs[12] = {std::pair<int,int> pairs[] = {std::pair<int,int> pairs[] = {
std::make_pair( 3,4 ),
std::make_pair( 4,9 ),Context
StackExchange Code Review Q#4209, answer score: 8
Revisions (0)
No revisions yet.