How can I improve and shorten this block of code?
You can improve you code in following steps:
- When there are multiple if statements and all have same structure you probably need to use an object
- You need to check of both uppercase and lowercase. Just use
toLowerCase()
on input. - You can
split
the string andmap()
it rather than creating an arraypush()
values into it.
function pairDNA(dna) {
const obj = {
c: 'CG',
g: 'GC',
t: 'TA',
a: "AT"
}
return dna.split('').map(x => obj[x.toLowerCase()])
}
If the string could contain anything other the specific letters then you need to filter()
the undefined
values after map
return dna.split('').map(x => obj[x.toLowerCase()]).filter(x => x !== undefined)
Another better is mentioned by @RobG in the comments that we can remove the unwanted letters from string before looping through it.
return dna
.toLowerCase()
.replace(/[^cgta]/g,'')
.split('')
.map(x => obj[x])
You can use a lookup mapping to simplify the loop:
function pairDNA(dna) {
const pairs = [], key = { G: "GC", C: "CG", A: "AT", T: "TA" };
for (let i = 0; i < dna.length; i ++)
pairs.push(key[dna[i].toUpperCase()]);
return pairs;
}
I'd probably:
Use a
for-of
loop (or possibly mapping with possible filtering)Use a lookup object or Map
Make the string lower or upper case when switching/looking up (but duplicated entries in the switch/lookup work too):
If you know that dna
will only ever contain c
/C
, g
/G
, t
/T
/, or a
/A
(which, as I understand it, is true of DNA ;-) ), then you can use Array.from
with its mapping feature with a lookup object/Map:
const table = {
c: "CG",
g: "GC",
t: "TA",
a: "AT"
};
function pairDNA(dna) {
return Array.from(dna, entry => table[entry.toLowerCase()]);
}
I'm using Array.from
because it will split the string on code points, not just code units (doesn't break up surrogate pairs) and has a mapping feature if you provide a mapping function. (Basically, Array.from(str, mappingFunction)
is [...str].map(mappingFunction)
but without the intermediate array.) Probably not all that relevant here given the content of your string, but can matter if your string may contain surrogate pairs.
Or with a Map
:
const table = new Map([
[c, "CG"],
[g, "GC"],
[t, "TA"],
[a, "AT"]
]);
function pairDNA(dna) {
return Array.from(dna, entry => table.get(entry.toLowerCase()));
}
If you can't make that assumption, add .filter
to filter out the ones that didn't have a match:
function pairDNA(dna) {
return Array.from(dna, entry => table.get(entry.toLowerCase())).filter(Boolean);
// or if using an object: return dna.map(entry => table[entry.toLowerCase()]).filter(Boolean);
}
Or if you want to avoid creating the extra array the filter
would create, stick with for-of
(or even your for
):
const table = {
c: "CG",
g: "GC",
t: "TA",
a: "AT"
};
function pairDNA(dna) {
const pairs = [];
for (const entry of dna) {
const value = table[entry.toLowerCase()];
if (value) {
pairs.push(value);
}
}
return pairs;
}
Maybe not shortened but definitely more maintainable.
function pairDNA(dna) {
const map = {
C: 'CG',
c: 'CG',
G: 'GC',
g: 'GC',
T: 'TA',
t: 'TA',
A: 'AT',
a: 'AT',
};
return dna.split('').reduce((tmp, x) => {
if (map[x]) {
tmp.push(map[x]);
}
return tmp;
}, []);
}
You could also do :
function pairDNA(dna) {
const map = {
c: 'CG',
g: 'GC',
t: 'TA',
a: 'AT',
};
return dna.split('').reduce((tmp, x) => {
if (map[x].toLowerCase()) {
tmp.push(map[x]);
}
return tmp;
}, []);
}