ahwkuepper / mcluster

McLuster - a tool to make a star cluster
21 stars 26 forks source link

Adding 150 characters to all the buffers #12

Open cmaureir opened 6 years ago

cmaureir commented 6 years ago

Using 50 is really low, and in modern times we do not need to care much, then using 200 is totally fine.

Issue-number: 4

Signed-off-by: Cristian Maureira-Fredes cmaureirafredes@gmail.com

teuben commented 6 years ago

Changing 50 to 200 is NOT the best solution:

1) at least use a #define for that lenght, and use that throughout 2) closer examination of the code should tell you how to use malloc() or something like that to use the correct amount.

I have a repo copy in https://github.com/teuben/mcluster which uses the following technique:

//Open output files
char *PARfile, *NBODYfile, *SSEfile;
FILE *PAR, *NBODY, *SSE12;
PARfile = strcat(output,".input");
PAR = fopen(PARfile,"w");
NBODYfile = strcat(output,".fort.10");
NBODY = fopen(NBODYfile,"w");

look at the start of main.c

This type of code is repeated over and over again, and could easily be uplifted in some utiity code so this doesn't have to be retyped all over the code. There was another issue on code-restructuring, which might involve this as well.

I thought I had made a pull request from my version, but seems not. Needs to be done or discussed what do to.

cmaureir commented 6 years ago

Well, I want it to be minimalistic as possible, this piece of code can be optimised in many ways. I have rewritten the whole code to separate functions, creating structures, and many others things, but I cannot open a pull request that changes everything, that is why I monkey-patched the small open issues, and I was trying to submit changes little by little.

I think C is no the best way of doing things, so I have also a set of patched that move the whole thing to C++, not using classes, because that will scare people, but just using all the perks of C++ standard library.

So yeah, sorry for just increase the size of the buffers, but big changes are usually slow to get merge :)

cmaureir commented 6 years ago

This is the branch that I currently have with the whole restructuration of the project https://github.com/cmaureir/mcluster/tree/improvement

I'm more than happy to hear any further concern regarding the code, or maybe another important aspect that I'm missing.