4
\$\begingroup\$

So my code is done it outputs exactly what it needs to I'm just wondering if it is possible to make this code a lot more simple using objects. If so could someone tell me what I would need member-wise and I will try and implement it. When I tried it got way too complex and it ended up looking easier to just write functions and a whole ton of for loops with if statements.

The purpose of the code is to analyze 2 inputs (base and template) and then see if the template matches any chunks on the base. Also called Gattaca, but A goes to T, C goes to G, and vice versa.

Here is my code:

#include <iostream>
#include <string>
using namespace std;

bool AT(char a, char b)  {
    return ((a == 'A' && b == 'T') || (b == 'A' && a == 'T'));
}
bool CG(char a, char b)  {
    return ((a == 'C' && b == 'G') || (b == 'C' && a == 'G'));

}
bool match(char a, char b) {

    return (AT(a, b) || CG(a, b));
}

int main() {
int case_count = 0;
string base;
string temp;
int base_size;
int temp_size;
int count;
cin >> case_count;

for (int i = 0; i < case_count; ++i) {
    cout << "Case " << i << ":\n";
    bool check = false;
    cin >> base;
    cin >> temp;
    base_size = base.size();
    temp_size = temp.size();

    if (base_size < temp_size) {
        cout << "None";
        continue;
    }
    if (base_size == temp_size) {
        count = 0;
        for (int index = 0; index < base_size; ++index) {
            if (!(match(base[index], temp[index]))) {
                cout << "None\n";
                break;
            }
            else 
                count++;

        }
        if (count == temp_size) {
            cout << "0\n";
        }
        continue;
    }
    for (int j = 0; j < base_size - (temp_size+1); ++j) {
        count = 0;
        for (int k = j; k < j + temp_size - 1; ++k) {
            if (!(match(base[k], temp[count]))) {
                break;
            }
            else
                count++;
        }
        if (count == temp_size - 1) {
            check = true;
            cout << j << " ";
        }

    }
    if (check == false) {
        cout << "None \n";
    }
    else if (check == true) {
        cout << "\n";

    }
}
return 0;
}

I'm new to C++ programming so the best I can think is using a class to make this look simple. I'm not looking for a full written program just an idea/pseudo code or just the member functions I could use to make this simple.

\$\endgroup\$
  • \$\begingroup\$ I've added the homework tag to indicate that you would rather not receive a complete solution. \$\endgroup\$ – 200_success Nov 3 '15 at 17:54
2
\$\begingroup\$

It is extremely difficult to understand what's going on in your code. Try to think of things the following perspective: the goal of programming is to tell other people what you want the computer to do. And what you want to do is match the template to the base as some location. So how can we make that clearer?

Rather than character-wise matching for the appropriate inversion, we can just invert the template and then we just have to match for equality. Suppose we have a function:

std::string invertDNA(std::string const& );

which flips A/T and C/G. Once we have that inversion, we're just searching for the inverted match_template in base:

std::size_t index = base_template.find(invertDNA(match_template));
if (index == std::string::npos) {
    std::cout << "None\n";
}
else {
    std::cout << index << '\n';
}

We've now reduced dozens of lines of loops and variables, which are error prone, into a one line standard algorithm which is very easy to both reason about and for other users to understand. I don't know anything about chemistry, but I get this!


For minor comments, avoid using namespace std; and keep your indentation consistent. You do not need variables for the string length, since you can always call .size(). Avoid comparing against true and false. That part can be if (!check) { ... } else { ... }

\$\endgroup\$
  • \$\begingroup\$ Thanks for the input! This gives me some better understanding of how to do it. So would you say it would be easier to just write a function that inverts it rather than creating a class and inverting it? \$\endgroup\$ – Motorscooter Nov 3 '15 at 19:21
  • \$\begingroup\$ @Motorscooter What would the class do for you? \$\endgroup\$ – Barry Nov 3 '15 at 19:25
  • \$\begingroup\$ so I implemented your code and I have it almost working my issue is now I'm trying to figure out how to iterate through to find all possible matches in the single base template do you have any ideas while I'm searching the internet? \$\endgroup\$ – Motorscooter Nov 4 '15 at 1:28
  • \$\begingroup\$ Scratch that I found out more about the find function from the string class and was able to write a while loop to do it. Thanks again for your input! \$\endgroup\$ – Motorscooter Nov 4 '15 at 2:12
0
\$\begingroup\$
  1. Try and use a more standard C++ style, to make your code more readable.
  2. Use more meaning full function and variable names, AT, CG and temp need revising.
  3. Always initialised variable at declaration, it will save you grief.
  4. "if(false) else if (true)" bools can only be true or false, so the second if is not necessary. In fact you could remove "\n" from the first clause (leaving just "None ") and then get rid of the else altogether.
  5. You have pre-incremented your loop variables, which is good practice, but then done count++;
  6. You could have validated case_count.
  7. Commenting the code makes it much easier to understand.

You need a constructor and destructor. Two constant functions AT and CG which call a common private function that refactors all the repletion there. Match can be constant too. And you need a function to do whatever main is doing.

Hope that helps

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.