The LED needs 20mA for full brightness and at that current has a forward voltage drop of 2.0V. The transistor I am using, 2N3904 has a saturation voltage of 0.2V. Also, I am using a 5V supply. So the collector resistor comes out to be Rc=(5-2-0.2)/(20*10^-3) = 140Ohm.
Now the minimum Hfe specified on the datasheet of the 2N3904 is 30, so I used that to find the base current as Ib = 20/30 = 666uA. Assuming Vbe of 0.7 volt, the base resistor can be found to be Rb = 6456Ohm. I can use the closest 6.2k.
Yes, that all looks OK, very thorough.
A few wrinkles you may want to consider.
hFE=30 is quite conservative for a small signal transistor. hFE varies a bit with collector current, you may find that it's more at the particular current you're running at. However, there's no harm to running a high base current, as long as it doesn't exceed base current and transistor dissipation limitations. Looking at the (ON semi) data sheet, hFE is 100min for Ic=10mA, and 60min for Ic=50mA.
Though you're using a switch to provide the base current, if you were to use a logic or uC output pin, there would be an additional drop which ought to be negligible at that current for CMOS families, but might be significant with old fashioned TTL, or an op-amp with non-R2R output.
Check the maximum current permitted for the LED. All things have a tolerance, your 5v supply could be higher, your 140ohm resistor could be lower, and you might find that with VCEsat lower than max as well, the LED current is higher than you expect. LEDs are very easy to see, not like a filament lamp whose brightness changes strongly with temperature, and you might see very little perceptual difference between 10mA and 20mA drive. 220 and 330ohm are common choices for the dropper from 5v to a red LED, and give currents comfortably below 20mA.