Hot answers tagged

29

Indeed you must have been working on this project for a while, since I answered a question on SO related to one of your old versions a while back. At least some of the feedback I had is still pertinent even now. Code and style comments #include <stdio.h> #include <stdlib.h> #include <math.h> #include <string.h> #include <time.h&...


20

A few things jump out at me immediately: Use sizeof() whenever you need the size of something with non-dynamic allocation. In other words, all of your write calls on the WaveHeader struct members should be using sizeof, not hard coded sizes. Allocation and initialization should be separate concerns. There's no need to have the same method that initializes a ...


17

You completely ignored the first points from the review your earlier question, and they still apply here: Whitespace is not a precious resource: your code is far too dense for my taste fewer semicolons, more newlines. try to limit your line length to 90 chars for readability I would go even further, and recommend to stay within 70 chars. ...


17

I think this should work. The key (pun intended) to making this work is to take advantage of the tag property of UIButton. In Interface Builder, set a different tag to each piano key. Start with 0 for "C3", 1 for "C#", up to 12 for "C4". Wire all of those keys to playKey. import UIKit import AVFoundation class ViewController: UIViewController { ...


17

Endianness Your program works on little endian machines only. If you care for it to work on any machine, you would need to fix up all of the fwrite() calls to output specifically in little endian format. Use struct for header I would suggest creating a struct for the file header. That way, you could fill in all the fields (in an endian correct manner) ...


15

If it walks like an array and talks like an array, use it like an array. float avg(float *data, size_t length) { float sum = 0; for (size_t i = 0; i < length; i++) { sum += fabs(*(data + i)); } return (float) sum / length; } So, data is an array, right? Why don't we treat it like one and access it using subscripts? float ...


15

fopen should have a b in the second parameter. This primarily prevents windows from replacing any \n byte with \r\n which would corrupt the file. fopen( fileName, "wb" ) Make the order of the fwrite parameters consistent first the sizeof and then the number of elements: // Writing the RIFF header: fwrite(&fChunkID, sizeof(fChunkID), 1, fout); ...


13

In C, I'd rather accept a pointer to a struct to be initialized than return newly allocated heap memory. It's clear that the caller owns the memory. The caller has the flexibility to pass you a pointer to a struct on its stack. void initWAVHeader(WaveHeader *hdr, uint32_t sample_rate, uint16_t bit_depth, uint16_t channels); The same goes for the rest of ...


13

Well, your post is over a year old, but hopefully this can still be of some use to you! Javadoc No need to say the method name again - the method name is automatically part of the Javadoc. What are the parameter constraints? (To be fair, you don't enforce any constraints in your code, so it makes sense to not mention any in your Javadoc) That said, you ...


13

This is very much C-style code: the only thing that qualifies it as C++ is the fact that you use new and delete[] rather than malloc() and free(). I don't see why RECORD_TIME should be a constant. I would expect it to be a constructor parameter. Or, use std::vector to store the samples, and you won't even have to worry about the array capacity, or even new ...


12

Layout Calling .setSize() with fixed dimensions isn't portable. For example, this is what happens on Mac OS X: Instead, call frame.pack() to automatically set the frame size according to its contents. If you want the 2 × 2 layout, then use a suitable LayoutManager — for example, p.setLayout(new GridLayout(2, 0, 10, 6)); Don't Repeat Yourself The code ...


11

Use MIDI Why play a WAV file, when you could synthesize the tones using MIDI? The tones can be played with indefinite length. You wouldn't need to distribute supplementary files with the program. You can also easily modify the program to generate other pitches without having to obtain new WAV recordings. import java.awt.BorderLayout; import java.awt....


11

Few notes. First, you don't want to copy data in the loop. Call memset()/memcpy() instead. You need some very serious reason to not use standard library. Here is some example code: dataSize = sizeof(*wptr) * data->numberOfChannels * framesToCalc; if (!inputBuffer) memset(wptr, dataSize, 0); else memcpy(wptr, rptr, dataSize); Second, recordFLAC(...


11

Design review Continuous reallocation of data.recordedSamples doesn't sound right. It may eventually fail because you require not just a lot of memory, but a lot of contiguous memory. Consider at least reusing data.recordedSamples instead of freeing it. That would require just one counter (how many blocks it may currently accomodate) and reduce amount of ...


11

Goto conditions are often maligned as being bad practice... and, for a reason. In almost every situation where a goto is used, it's because you are doing too much in a method. Consider your main method: int main(void) { PaError err = paNoError; if((err = Pa_Initialize())) goto done; ..... AudioData data = initAudioData(44100, info->...


11

It actually doesn't look too bad. Some minor nitpicks You shouldn't cast the return value of malloc. fByteRate and fBlockAlign can be const since they're being computed from const values only. I'm not entirely sure why pretty much all variables in WriteWavePCM are prefixed with f. I suppose this is meant to indicate that they are related to data being ...


10

Things you did well Nicely formatted, easy to read. Use of typedef with structures. Things you could improve Preprocessor: Since SDL.h isn't one of your own pre-defined header files, you should be searching for it in directories pre-designated by the compiler (since that is where it should be stored). #include <SDL/SDL.h> In the C standard, §6.10....


10

You can clean up the repetition by moving the ActionListener's code into a custom class, like this: static class KeyButtonListener implements ActionListener { private final JButton button; private final String filename; KeyButtonListener(JButton button, String filename) { this.button = button; this.filename = filename; } ...


10

import javax.swing.*; import javax.sound.midi.*; import java.awt.*; import java.awt.event.*; Most style guides will frown upon the wildcard import -- is it so much extra work to list the classes you actually depend on? This will also help you to avoid conflicts, where you want List to mean java.util.List rather than java.awt.List public static ArrayList&...


10

First, you aren't using braces around one-line if statements: if (!outfile) return -1; Failing to use braces was the cause of Apple's SSL bug, as described here. Why are you doing this? #define FRAMES_PER_BUFFER 1024 Shouldn't that be static const int FRAMES_PER_BUFFER = 1024;? This SO question discusses this. As @RubberDuck mentioned in the comment, ...


10

I have to admit that I have no idea what the purpose of the code is (never worked with speech recognition) but I know a little bit Python so I can give some advise on the code itself. Performance You expressed the desire to optimize the performance of your script. But do you know the first rule about optimization? If not, it is: Don't! The second rule is:...


9

To generate those nested, nearly identical setTimeout calls, instead of coding them all manually you could have a recursive function that builds and returns a function for you. Something like this would work: // Accepts an array of elements and a final callback function // Returns a function to show all elements and execute the callback function ...


9

In addition to the excellent advice given in the other answers, I'd recommend completely overhauling this loop: for (i = 0, j = 0; i < N_SAMPLE_PAIRS * N_CHANNELS; i += 2, j++ ) { short datum1 = 450 * ((j >> 9 | j >> 7 | j >> 2) % 128); short datum2 = 450 * ((j >> 11 | j >> 8 | j >> 3) % 128); sound[i] ...


8

Your recordWAV is supposed to receive a header that has been setup with genericWAVHeader. That function takes the desired sample rate and applies it to the header. However, within recordWAV, the functions that setup the PCM handle parameters determine the real sample rate (your call to snd_pcm_hw_params_set_rate_near modifies the suggested sample rate) and ...


8

Let me start by saying I have very limited Android experience. That said, I did look into the SoundPool class a bit and have some questions/comments about your implementation: If you aren't already aware of some of the reasons why people consider Singletons an anti-pattern, I suggest you do some research on the topic and make sure you're ok with the ...


8

I totally agree with Ocelot20, but wanted to point out some more things and provide some comments of my own about some things that has been already mentioned: About mAvailableSounds: Vector is deprecated, use List (interface) and ArrayList (implementation) instead. private List<Integer> mAvailibleSounds = new ArrayList<Integer>(); Does the ...


8

No reason to pack a lot of conditions in the same if: if (!sound || !fileName || !(fout = fopen( fileName, "w" ))) Could very well be: if (sound == NULL || fileName == NULL) { return false; } FILE * fout = fopen(fileName, "wb"); if (fout == NULL) { return false } Personal preference, but like to avoid assignments inside conditional, they ...


8

Really interesting thing you've created! I've never used ChucK, but I'll try to comment on some things. I also have some, but limited music theory knowledge. If language constraints is a reason for doing something I've mentioned below, please excuse my ignorance :) Semitone ratio? I wonder why you chose to create the getSemitoneRatio function. It's only ...


7

Overall it looks fine for a newcomer, you've made proper use of const for variables and methods, which is something beginners tend to overlook. A few points you can still work on: For such a tiny class like Tone, I would not bother separating declaration from implementation and would have declared it inline in the header file only. That would be less ...


7

Key-to-frequency mapping Characters with an ASCII code that is a multiple of 20 (such as d) are mapped to 0 Hz, resulting in silence. You wrote new Noise(c % 20 * 110) in two places, so fixing this problem will require changing both call sites. To avoid such duplication, it would be better to have a Noise(char c) constructor that chains to the Noise(int ...


Only top voted, non community-wiki answers of a minimum length are eligible