Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Csv export #131

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Csv export #131

wants to merge 3 commits into from

Conversation

shaybrow
Copy link
Collaborator

@shaybrow shaybrow commented Jun 9, 2021

No description provided.

Copy link
Collaborator

@wordhou wordhou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good start. Two things that come to me right away:

We should try and separate the logic for actually producing the CSV with the logic for handling the files and attaching them to an email, since they are on different levels. (the first is business logic and the second is implementation logic). This may be kind of difficult with the csv library that you've chosen. We may want to store our CSV data in an intermediate format, like a List<List>.

Also we discussed this over discord but we should be able to replace the external file system access with internal file system access.

Comment on lines +27 to +49
public void convertToCsv (Batch batch) throws IOException {
String path = baseDir + File.separator + "BatchData.csv";
File f = new File(path);
if (f.exists()&& f.isDirectory()){
csvWriter= new CSVWriter(new FileWriter(path, true));
} else{
csvWriter= new CSVWriter(new FileWriter(path));
}

// still need to use intent to save file or email
List<Measurement> batchMeasurements = batch.getMeasurements();
csvWriter.writeNext(new String[] {"Type", "Number", "Identifier", "Created At", "Updated At"});
csvWriter.writeNext(new String [] {batch.getType(), String.valueOf(batch.getBatchNumber()), batch.getBatchIdentifier(), batch.getCreatedAt().toString(), batch.getUpdatedAt().toString()});
csvWriter.writeNext(new String [] {""});
csvWriter.writeNext(new String [] {"True Proof", "Temperature", "Temp. Correction", "Hydrometer", "Hydro Correction", "Created At"});
for (Measurement m : batchMeasurements
) {
csvWriter.writeNext(new String [] {String.valueOf(m.getTrueProof()), String.valueOf(m.getTemperature()),
String.valueOf(m.getTemperatureCorrection()), String.valueOf(m.getHydrometer()), String.valueOf(m.getHydrometerCorrection())
, m.getCreatedAt().toString()});
}
csvWriter.close();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we want this method to be side-effect free and return something, rather than returning void and doing side effects.

My thoughts are to either make this return a String with the contents of the CSV file. Or, if we want to return a file, create the file without Android's API and return a URI.

Comment on lines +5 to +6
<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" />
<!-- requests write to storage permission for csv export-->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe we actually need this permission to create a file and attach it to an email. I believe we can accomplish our task without this.

// +"\",\"" + batch.getCreatedAt().toString()+"\",\"" + batch.getUpdatedAt().toString()
String combinedString = batchHeader + "\n" + batchInfo;
File file = null;
File root = Environment.getExternalStorageDirectory();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if we can use internal storage rather than external storage so we don't need to request permissions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants