ejh243 / BrainFANS

Complex Disease Epigenomics Group's quality control and analysis pipelines for DNA methylation arrays, SNP arrays, BS-Seq, ATAC-Seq and ChIP-Seq
Other
10 stars 4 forks source link

refactor: convert section "definPlotParams" into functions #178

Closed sof202 closed 2 months ago

sof202 commented 2 months ago

Description

This pull request:

Issue ticket number

This pull request is to address issues: #173 and #177.

Type of pull request

Checklist

sof202 commented 2 months ago

Current problems with my code

One

If both techVar and bioVar are undefined, then plotCols will just be the Basename column of QCmetrics. Is this the desired effect? (This is what the code did before, just checking if this is actually what we want.)

Two

Not sure if I like the if statement hell:

if (all(is.na(column))) return(FALSE)
if (length(unique(column)) == 1L) return(FALSE)
if (anyDuplicated(column) == 0L) return(FALSE)
return(TRUE)

I could change this to:

if (
all(is.na(column)) || 
length(unique(column)) == 1L || 
anyDuplicated(column) == 0L) {
  return(FALSE)
} 
return(TRUE)

Or:

return(!(all(is.na(column)) || 
          length(unique(column)) == 1L || 
          anyDuplicated(column) == 0L))

Though I'm not convinced these are much cleaner. I'm open to suggestions

sof202 commented 2 months ago

Not sure if I like the if statement hell:

if (all(is.na(column))) return(FALSE)
if (length(unique(column)) == 1L) return(FALSE)
if (anyDuplicated(column) == 0L) return(FALSE)
return(TRUE)

I could change this to:

if (
all(is.na(column)) || 
length(unique(column)) == 1L || 
anyDuplicated(column) == 0L) {
  return(FALSE)
} 
return(TRUE)

Or:

return(!(all(is.na(column)) || 
          length(unique(column)) == 1L || 
          anyDuplicated(column) == 0L))

Though I'm not convinced these are much cleaner. I'm open to suggestions

I've chosen the 3rd option now