patterncMinor
Counting the longest increasing subsequence in an array of integers
Viewed 0 times
thecountingarraysubsequenceintegersincreasinglongest
Problem
This is my finished function to find the length of the longest consecutive subsequence of an input array of integers. It's for an assessment so things like the numerical method used and the format of input are out of my hands (
As far as I can tell works perfectly, however, our lecturer has barely commented on how to write code that doesn't just do the job but is 'good code' (aka a large amount of the marking criteria).
Is there anything that you can see which could be improved here?
In the past I've lost marks for code that did the job, but didn't follow 'good practice' standards. I've tried to make this code as nice as I can, with a clear logical structure, good commenting etc but I always thought my code was good then because when I tested it it worked. I'm just looking for advice on the form of the code.
int LIS(int seq, int temp_seq, int seq_size)). For the temp_seq parameter, the caller will pass an empty array of size seq_size.As far as I can tell works perfectly, however, our lecturer has barely commented on how to write code that doesn't just do the job but is 'good code' (aka a large amount of the marking criteria).
Is there anything that you can see which could be improved here?
In the past I've lost marks for code that did the job, but didn't follow 'good practice' standards. I've tried to make this code as nice as I can, with a clear logical structure, good commenting etc but I always thought my code was good then because when I tested it it worked. I'm just looking for advice on the form of the code.
int LIS(int* seq, int* temp_seq, int seq_size)
{
/*sets up variables and initial values*/
int i, proceed_flag;
int j = 0, count = -1;
/*this'll be used to ensure each input element is only used once - 1 means not used*/
int use_flag [seq_size];
for (i = 0; i seq[i] && use_flag[i])
{
temp_seq[j+1] = seq[i];
use_flag[i] = 0;
// printf("temp_seq = %d\n", temp_seq[j+1]);
j++;
}
}
/*this counts the number of repeats - the initialisation value of -1 is just to calibrate the system
it takes an extra loop around and gives a wrong result otherwise*/
count++;
}
return(count);
}Solution
Here is one comment about the readability of your code.
I allowed myself to change the "use" to "used", because it is more grammatically correct.
Now first of all, if "1 means not used", then you should name this array
Alternatively, you can keep calling it
If you choose the second option, then you can initialize the entire array upon declaration:
Please note that it doesn't yield better performance because it's a non-static local array.
Nevertheless, it does clean up your code a bit...
Another point is implied in a comment at the bottom of your code:
It sounds like an ugly workaround for "an extra loop" that should not take place.
So I think that you definitely need to solve it properly instead of working around it.
Last issue (though possibly related to the previous one):
By entering the loop with
Your code might be working correctly for some input, but beware that it does look suspiciously unsafe.
/* 1 means not used */
int used_flag[seq_size];I allowed myself to change the "use" to "used", because it is more grammatically correct.
Now first of all, if "1 means not used", then you should name this array
not_used_flag.Alternatively, you can keep calling it
used_flag, but let 0 denote the case of "not used".If you choose the second option, then you can initialize the entire array upon declaration:
int used_flag[seq_size] = {0};Please note that it doesn't yield better performance because it's a non-static local array.
Nevertheless, it does clean up your code a bit...
Another point is implied in a comment at the bottom of your code:
/* the initialization value of -1 is just to calibrate the system.
it takes an extra loop around and gives a wrong result otherwise. */It sounds like an ugly workaround for "an extra loop" that should not take place.
So I think that you definitely need to solve it properly instead of working around it.
Last issue (though possibly related to the previous one):
while (j < seq_size)
{
...
temp_seq[j+1] = ...;
}By entering the loop with
j == seq_size-1, you'll be writing beyond the boundaries of temp_seq.Your code might be working correctly for some input, but beware that it does look suspiciously unsafe.
Code Snippets
/* 1 means not used */
int used_flag[seq_size];int used_flag[seq_size] = {0};/* the initialization value of -1 is just to calibrate the system.
it takes an extra loop around and gives a wrong result otherwise. */while (j < seq_size)
{
...
temp_seq[j+1] = ...;
}Context
StackExchange Code Review Q#68667, answer score: 3
Revisions (0)
No revisions yet.