{"action":"opened","issue":{"url":"https://api.github.com/repos/an2netto/02-P-Dev_Tools/issues/1","repository_url":"https://api.github.com/repos/an2netto/02-P-Dev_Tools","labels_url":"https://api.github.com/repos/an2netto/02-P-Dev_Tools/issues/1/labels{/name}","comments_url":"https://api.github.com/repos/an2netto/02-P-Dev_Tools/issues/1/comments","events_url":"https://api.github.com/repos/an2netto/02-P-Dev_Tools/issues/1/events","html_url":"https://github.com/an2netto/02-P-Dev_Tools/issues/1","id":339646256,"node_id":"MDU6SXNzdWUzMzk2NDYyNTY=","number":1,"title":"Some notes","user":{"login":"thodnev","id":16870636,"node_id":"MDQ6VXNlcjE2ODcwNjM2","avatar_url":"https://avatars1.githubusercontent.com/u/16870636?v=4","gravatar_id":"","url":"https://api.github.com/users/thodnev","html_url":"https://github.com/thodnev","followers_url":"https://api.github.com/users/thodnev/followers","following_url":"https://api.github.com/users/thodnev/following{/other_user}","gists_url":"https://api.github.com/users/thodnev/gists{/gist_id}","starred_url":"https://api.github.com/users/thodnev/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/thodnev/subscriptions","organizations_url":"https://api.github.com/users/thodnev/orgs","repos_url":"https://api.github.com/users/thodnev/repos","events_url":"https://api.github.com/users/thodnev/events{/privacy}","received_events_url":"https://api.github.com/users/thodnev/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2018-07-09T23:54:11Z","updated_at":"2018-07-09T23:54:11Z","closed_at":null,"author_association":"NONE","body":"First things first: your code is good. Very good.\r\nSome minor notes below\r\n\r\nhttps://github.com/an2netto/02-P-Dev_Tools/blob/06c2ccf51164c39498ea49ad5a4717893c0358a8/dft.c#L3\r\nIf we talk about precision, it's better not to use `.f` qualifier here. `acos` accepts double. And moreover, if it's a constant, it could be precisely calculated elsewhere and pasted as `const double tau = 6.2831853071795864769252867665590057683943387987502;`\r\n\r\nThat's DFT, not pure fft. As fft uses exp-table and complex arithmetic. See [butterfly](https://en.wikipedia.org/wiki/Butterfly_diagram) approach\r\n\r\nhttps://github.com/an2netto/02-P-Dev_Tools/blob/06c2ccf51164c39498ea49ad5a4717893c0358a8/dft.c#L5\r\nNo need to use `float` here. `double` is more precise and faster on x86 https://stackoverflow.com/a/4584707/5750172\r\n\r\nhttps://github.com/an2netto/02-P-Dev_Tools/blob/06c2ccf51164c39498ea49ad5a4717893c0358a8/dft.c#L7\r\n[CodingStyle](https://github.com/an2netto/02-P-Dev_Tools/files/2178320/CodingStyle1.pdf) requires whitespaces\r\n\r\nIf we talk about computation-heavy tasks like FFT, it's better to use existing libs (fftpack, fftw).\r\n\r\nhttps://github.com/an2netto/02-P-Dev_Tools/blob/06c2ccf51164c39498ea49ad5a4717893c0358a8/dft.h#L12\r\nIt still depends on your personal preferences, but I recommend not specifying the argument names in prototypes. So that if you ever want to change them further (during refactoring, etc.), you'll have a single place for changes. (Yes, argument names in doc comments still should be changed, but that's less important in the end)\r\n\r\nhttps://github.com/an2netto/02-P-Dev_Tools/blob/06c2ccf51164c39498ea49ad5a4717893c0358a8/dft.h#L12\r\nErrors could arise during fft computation (from mathematical perspective in this particular case). It's good to have func returning pretty enum errorcode.\r\nAlso the complex type could be used in function.\r\n\r\nhttps://github.com/an2netto/02-P-Dev_Tools/blob/06c2ccf51164c39498ea49ad5a4717893c0358a8/dft_main.c#L25-L30\r\nNo need to declare variables at the top. It's much better to declare them somewhere nearby the place they're first used, i.e. \r\n```C\r\nfloat sig = malloc(size * sizeof *sig);\r\n```\r\n\r\nAlso the `malloc` and other functions could return errors. For `malloc` it happens, for example, when no free memory is available in pool (imagine you're working on embedded ARM target with 32 MB of ram). That's why obscure checks are usually used in embedded, like:\r\n```C\r\nif ((int *val = emb_malloc(sizeof *val)) == NULL) {\r\n        errlvl = EALLOC;\r\n        goto val_free;  // preserve the order. alloc: a -> b -> c; free: c -> b -> a\r\n}\r\n```\r\n\r\nhttps://github.com/an2netto/02-P-Dev_Tools/blob/06c2ccf51164c39498ea49ad5a4717893c0358a8/dft_main.c#L53-L63\r\nIn production code, commented blocks should appear after description, what's the commented-out block does and why it's important to keep it in the code base\r\n\r\n"}}
{"actor":{"display_login":"thodnev"}}