No pasó la entrevista sobre una calculadora RPN -- ruby campo con interview-questions campo con calculator camp codereview Relacionados El problema

Did not pass the interview about an RPN Calculator


12
vote

problema

Español

Recientemente recibí una simple asignación de entrevistas para implementar una calculadora RPN en Ruby. El programa toma entrada de usuario y se reduce cuando el usuario presiona 'Q'.

Lo hice en menos de 30 minutos y confié en mi solución, pero los entrevistadores no les gustó. Me encantaría saber las fallas que tiene, por lo que puedo evitar estar demasiado confiado la próxima vez y aprender mejor las prácticas de codificación!

  #!usr/bin/env ruby  # A Ruby RPN Calculator  #Note: decimal numbers are rounded to nearest tenth =begin example input/ouput > 2 2 > 9 9 >3 3 > + 12 > * 24 =end  class RPNCalculator    attr_reader :operators   def initialize(operators)     @operators=operators   end    def start     print '> '     n = gets.strip     stack = []      while n!= 'q'       if n =~ /d/         stack.push(n.to_f)         puts n       elsif self.operators.include? n         operands = stack.pop(2)         res = operands.reduce(n.to_sym)         (res.to_i == res) ? (puts res.to_i) : (puts "%0.1f" % res)         stack.push(res)       else          puts "Invalid input"       end       print '> '       n = gets.strip     end   end end  rpn = RPNCalculator.new(["+","-","*","/"]) rpn.start   
Original en ingles

I recently received a simple interview assignment as to implement an RPN calculator in Ruby. The program takes user input and quits when the user presses 'q'.

I did it in under 30 minutes and was quite confident about my solution, but the interviewers did not like it. I would love to know the flaws it has so I can avoid being overconfident next time and learn better coding practices!

#!usr/bin/env ruby  # A Ruby RPN Calculator  #Note: decimal numbers are rounded to nearest tenth =begin example input/ouput > 2 2 > 9 9 >3 3 > + 12 > * 24 =end  class RPNCalculator    attr_reader :operators   def initialize(operators)     @operators=operators   end    def start     print '> '     n = gets.strip     stack = []      while n!= 'q'       if n =~ /\d/         stack.push(n.to_f)         puts n       elsif self.operators.include? n         operands = stack.pop(2)         res = operands.reduce(n.to_sym)         (res.to_i == res) ? (puts res.to_i) : (puts "%0.1f" % res)         stack.push(res)       else          puts "Invalid input"       end       print '> '       n = gets.strip     end   end end  rpn = RPNCalculator.new(["+","-","*","/"]) rpn.start 
        

Lista de respuestas

13
 
vote

No sé cómo se observó su asignación exactamente, por lo que algunos de los siguientes pueden ser las críticas equivocadas. Acabo de intentar empujar tantos agujeros en el código como sea posible; justo o de otra manera. Algo probablemente se superpondrá con los pensamientos de su entrevistador. Pero, por supuesto, no sé qué podría ser, por lo que no hay un giro real a estos puntos, simplemente los escribí como pensé en ellos.

  • Separe la lógica de la interfaz de usuario de la respuesta. Sería mejor si pudiera empujar la entrada a la calculadora desde cualquier lugar. Y sería mucho más fácil probar el código.

  • Hablando de: Las pruebas serían agradables de ver. Si el lugar que entrevistó es realmente grande en TDD / BDD, tal vez eso es lo que querían ver más que nada. Las asignaciones de entrevistas (cuando se hacen bien) son tanto sobre el proceso del candidato, ya que es el resultado.

  • PATH=something3 es un método bastante largo y lo hace bastante, bueno, todo . Si lo llamas, comienza un bucle, no podrá salir del código. Eso es un poco aterrador. El método simplemente hace demasiado.
    Una vez más, una estructura diferente ayudará. Además, las declaraciones 99887766555443314 son bastante poderosas. Podría ser útil en lugar de PATH=something5 .

  • El Regex usado para hacer coincidir los números es muy suelto. Aceptará cualquier cosa mientras haya un dígito en algún lugar en él. Por lo tanto, PATH=something6 es técnicamente un número en los ojos de su código.

  • El PATH=something7 Línea es varios tipos de desagradables. Sería mucho más limpio si pones el PATH=something8 primero, y usó el ternario para sus argumentos en su lugar. En este momento, es "Si un código PATH=something9, de lo contrario, si B, entonces ... Bueno, entonces también 99887776655443320 ". Además, no quiere usar las ramas de un ternario para nada, excepto las cosas más simples. Querrá usarlos para decidir entre los valores, no las acciones. Por lo tanto, llame a $IFS1 en cosas o haciendo una aritmética simple está bien, pero no vayas demasiado lejos. Si desea hacer más, use un 998877666555443322 .
    Algo como esto tendría más sentido:

      $IFS3  

    O podría usar $IFS4 en su lugar, que ignorarán decimales para enteros:

      $IFS5  

    Sin embargo, truncará grandes números, por lo que no es un ganador claro.

  • Su código acepta una lista de operadores, pero asume:

    1. que cualquier cadena que pase es realmente válida (de lo contrario, se romperá la llamada 99887766555443326555443326 )
    2. que todos los operadores tienen una aridad de 2.

    Entonces, el código es a mitad de camino configurable, y a mitad de camino. Supongamos que desea admitir un operador $IFS7655443327 para factorial: para uno, 99887766555443328 no es un método en $IFS9 , por lo que 99887776655443330 fallará. Para otro, el operador factorial solo tiene un operando, no dos.
    Francamente, no me molestaría de hacer este configurable a menos que me pidieran. O preguntaría si no estaba seguro, en lugar de hacer suposiciones.

  • Hablando de la aridad del operador: el script se bloquea si la pila está vacía y ingresa a un operador. Asumiste que todos los operadores siempre tendrían dos operandos, pero también asumiste que siempre son dos operandos.

  • No estoy seguro de cuál se supone que la última entrada de ejemplo ( 99887766555443331 ) se debe hacer. Pulsa 24 a la pila y multiplica? Quejarse de la entrada? (EDITAR: Como se señaló en los comentarios, es probable que solo esté perdiendo el alineado. Eso tendría sentido. Aunque debería haberlo descubierto yo mismo, acaba de darle la vuelta y decir que deberías leer los comentarios de su código y Documentación :)
    Independientemente de su significado previsto, es interesante como entrada de prueba: imprime local IFS=...2 de nuevo a usted, porque imprime la entrada en bruto, en lugar del valor analizado. Así que no sabes lo que realmente hace el script de su entrada. Sin embargo, lo que hizo, sin embargo, fue presionando local IFS=...3 a la pila, porque el código cree que es un número, pero 998877766554433334 solo devuelve cero.

  • Hablando de lo que imprime, echando eco de la entrada del usuario de vuelta es una especie de inútil. Quiero decir, el usuario acaba de escribirlo. Imprimir la pila (o al menos la entrada después de la interpretación) probablemente sería más útil.

  • nombramiento. La variable local IFS=...5 es demasiado corta para significar mucho. Supongo que significa "número", pero eso solo tiene sentido si la entrada es de hecho un número. Simplemente lo llamaría local IFS=...6 . Además, local IFS=...7 es unnect Ssily acortado. No es que malo escribir local IFS=...8 en su lugar. Realmente no hay razón para abreviar.
    De lo contrario, el nombramiento está bien, me gusta ese local IFS=...99 y mktemp0 son palabras completas. Aún así, como se mencionó, mktemp1 no es un gran nombre para un método que lo haga que incluya leer, evaluar, imprimir y (irónicamente) deteniéndose. Pero eso es menos una cuestión de cambiar el nombre; El método en sí debe refactorarse.

  • Si desea ser elegante, podría hacer una clase 9988777665554433422 , e implementar 99887776655443343 , 99887766555443344 , etc. como métodos en esa clase.

  • FRYE PEQUEÑO, PERO 99887776655443345 podría ser escrito más simplemente como mktemp6 . Pero de nuevo, saltaría de hacerlo configurable.

Ahora, como se mencionó, no sé cuál era su ejercicio exacto. Usted construyó una calculadora de Refle, ¡pero quizás quisieran algo que pudiera analizar una expresión completa como mktemp7 ? ¿O un "motor" de la calculadora que se puede llamar desde el código? Si ese es el caso, un poco fuiste la forma equivocada de comenzar con :)

Editar: Debería agregar que no tengo la intención de que esto sea severo. Como señala 200_success, no es nada malo para & lt; 30 minutos de codificación durante la presión.

 

I don't know how your assignment looked exactly, so some of the below may be misguided criticisms. I've just tried to poke as many holes in the code as possible; fair or otherwise. Something will probably overlap with your interviewer's thoughts. But of course, I don't know what that might be, so there's no real order to these points - I just wrote them as I thought of them.

  • Separate the logic from the REPL user interface. It'd be nicer if you could push input to the calculator from anywhere. And it'd be much easier to test the code.

  • Speaking of: Tests would be nice to see. If the place you interviewed is really big on TDD/BDD, maybe that's what they wanted to see more than anything. Interview assignments (when done well) are as much about the candidate's process as it is the result.

  • #start is a rather long method and it pretty much does, well, everything. If you call it, it starts a loop you won't be able to exit from code. That's kinda scary. The method just does too much.
    Again, a different structure will help. Also, Ruby's case statements are pretty powerful. Might come in handy instead of if...elsif...else.

  • The regex used to match numbers is very loose. It'll accept anything as long as there's a digit somewhere in it. So "u wot m8" is technically a number in the eyes of your code.

  • The (res.to_i == res) ? (puts res.to_i) : (puts "%0.1f" % res) line is several kinds of nasty. It'd be much cleaner if you put the puts first, and used the ternary for its arguments instead. Right now it's "if A then puts, otherwise if B then... well, then also puts". Also you don't want to use a ternary's branches for anything except the simplest things. You'll want to use them to decide between values, not actions. So calling to_i on stuff or doing simple arithmetic is fine, but don't go too far. If you want to do more, use a proper if..else.
    Something like this would make more sense:

    puts res.to_i == res ? res : "%0.1f" % res 

    Or you could use %g instead, which'll ignore decimals for integers:

    puts "%g" % res.round(1) 

    However, it'll truncate large numbers so it's not a clear winner.

  • Your code accepts a list of operators, but it assumes:

    1. that any string you pass is actually valid (otherwise the reduce call will break)
    2. that all the operators have an arity of 2.

    So the code is halfway configurable, and halfway not. Suppose you want to support a ! operator for factorial: For one, ! is not a method on Float, so reduce(n.to_sym) will fail. For another, the factorial operator only has one operand, not two.
    Frankly, I wouldn't bother making this configurable unless I was asked to. Or I'd ask if I was unsure, rather than make assumptions.

  • Speaking of operator arity: The script crashes if the stack's empty and you enter an operator. You assumed that all operators would always have two operands, but you also assumed that there always are two operands.

  • Not sure what the last example input (* 24) is supposed to do. Push 24 to the stack and multiply? Complain about the input? (Edit: As pointed out in the comments, it's probably just missing linebreak. That would make sense. Even though I should've figured it out myself, I'll just turn that around and say you should proof-read your code comments and documentation :)
    Regardless of its intended meaning, it's interesting as test input: It prints * 24 back to you, because you print the raw input, rather than the parsed value. So you don't know what the script actually made of your input. What it did, though, was push 0.0 to the stack, because the code thinks it's a number, but "* 24".to_f just returns zero.

  • Speaking of what you print, echoing the user's input right back is sort of pointless. I mean, the user just wrote it. Printing the stack (or at least the input after interpretation) would likely be more useful.

  • Naming. The variable n is too short to mean much. I suppose it means "number", but that only makes sense if the input is indeed a number. I'd just call it input. Also, res is unnecessarily shortened. It's not that bad to write result instead. There's really no reason to abbreviate.
    Otherwise, naming is fine - I like that operators and operands are both full words. Still, as mentioned, #start is not a great name for a method that does everything including reading, evaluating, printing and (ironically) stopping. But that's less a question of changing the name; the method itself should be refactored.

  • If you want to be fancy, you could make a Stack class instead, and implement #*, #+, etc. as methods on that class.

  • Small fry, but ["+","-","*","/"] could be written more simply as %w(+ - * /). But again, I'd skip making it configurable.

Now, as mentioned I don't know what your exact exercise was. You built a REPL calculator, but perhaps they wanted something that could parse a full expression like "3 4 * 2 /" => 6? Or a calculator "engine" that's callable from code? If that's the case, you kinda went the wrong way to begin with :)

Edit: I should add that I don't intend for this to be harsh. As 200_success points out, it's not at all bad for < 30 mins of coding while under pressure.

 
 
   
   
3
 
vote

Esto no es malo para el código de la entrevista escrito en las limitaciones de tiempo y la presión mental.

Mi primera observación es que el diseño del objeto es deficiente. No hay mucho sentido definir un objeto con un método. Usted podría haber escrito esto como una función. Si desea escribir mktemp8 como un objeto, entonces esperaría que la pila fuera una propiedad de la calculadora, no en el bucle de ejecución.

El comportamiento es algo extraño en la pila de un infriso. Espero que imprima algún tipo de mensaje de error y deje la pila sin cambios. Su código abandona la pila sin cambios (pero por una razón curiosa). Si da mktemp9 como la primera entrada, luego se bloquea con 'http://'+url0 .

El código está diseñado. Solo acomodan a los operadores que toman dos operandos y devuelven un número. No puede tener un operador 998877766555443351 , por ejemplo. Además, una calculadora de RPN útil admitiría operaciones de manipulación de pila, como 'http://'+url2 , 'http://'+url3 , 'http://'+url4 , 99887766555443355 , 'http://'+url6 , y 'http://'+url7 .

Para un código más claro y menos redundante, el código de solicitud debe extraerse del bucle. Yo escribiría ...

  'http://'+url8  

La expresión ternaria 'http://'+url9 es realmente un IFS OSSE. Estaría mejor escrito como urlunparse0 . (En mi opinión, un lugar decimal es insuficiente: ni siquiera puede hacer cálculos de dólares y centavos decentes).

La línea Shebang no tiene la derecha: debería ser urlunparse1 .

 

This isn't bad for interview code written under time constraints and mental pressure.

My first observation is that the object design is deficient. There isn't much point to defining an object with one method. You could just as well have written this as a function. If you were to write RPNCalculator as an object, then I would expect the stack to be a property of the calculator, not of the run loop.

The behaviour is somewhat odd on stack underflow. I would expect it to print some kind of error message and leave the stack unchanged. Your code does leave the stack unchanged (but for a curious reason). If you give + as the first input, then it crashes with in `%': can't convert nil into Float (TypeError).

The code is designed do accommodate only operators that take two operands and return one number. You cannot have a cos operator, for example. Also, a useful RPN calculator would support stack-manipulation operations such as clear, drop, dup, roll, rolld, and swap.

For clearer and less redundant code, the prompting code should be extracted out of the loop. I would writexe2x80xa6

def start   while (input = prompt('> ')) != 'q' do     xe2x80xa6   end end  private def prompt(s)   print s   gets.strip end 

The ternary expression (res.to_i == res) ? (puts res.to_i) : (puts "%0.1f" % res) is really an if-else. It would be better written as puts (res.to_i == res) ? res.to_i : "%f" % res. (In my opinion, one decimal place is insufficient xe2x80x94 you can't even do decent dollars-and-cents calculations.)

The shebang line is not quite right: It should be #!/usr/bin/env ruby.

 
 
 
 

Relacionados problema

4  Calculadora binaria de Python  ( Python binary calculator ) 
Mi tarea fue construir una calculadora en la adición binaria de soporte de Python, etc. Para comenzar Definir un par de excepciones personalizadas: paper...

10  Calculadora básica que lleva 2 números y hace una operación con ellos  ( Basic calculator that takes 2 numbers and does an operation with them ) 
El código funciona perfectamente bien para mí (lo compiló en Bluej y Eclipse), pero me preguntaba qué se consideraban otros programadores más experimentados. ...

6  Calculadora aritmética básica  ( Basic arithmetic calculator ) 
La calculadora admite los operadores básicos (agregar, sustancia, dividir, multiplicar). en este momento, solo funciona con enteros positivos No valido...

0  Calculadora de java simple en columpio  ( Simple java calculator in swing ) 
¿Puedes revisar este código para mí? ¿Cómo lo hago mejor? //this is the calculator import java.awt.*; import java.awt.event.*; import javax.swing.*; import...

8  Calculadora de entrada de 2 entradas Java GUI  ( Java gui 2 input calculator ) 
He escrito este código para una calculadora de dos entradas. ¿Hay algo que pueda hacer para que este código sea más eficiente? import javax.swing.*; impor...

2  Una calculadora básica en C que utiliza un bucle  ( A basic calculator in c that uses a loop ) 
He creado un programa que forma una calculadora básica en C que incluye las operaciones básicas como la adición, la resta, la multiplicación y la división. Pe...

6  Calculadoras para ecuaciones de la ley de gas  ( Calculators for gas law equations ) 
He realizado un programa C ++ que calcula una variable faltante en una ecuación para una de las cinco leyes siguientes de gas: la ley de Boyle ley de car...

5  Calculadora de área y volumen  ( Area and volume calculator ) 
Soy un codificador para principiantes, haciéndolo únicamente por diversión, habiendo comenzado a codificar hace unos dos meses con Python. Tengo un punto de t...

6  Calculadora simple en C #  ( Simple calculator in c ) 
Es una calculadora básica donde el usuario ingresa a dos números, y una operación y el programa lo convierten en una ecuación y obtiene la respuesta. Por ejem...

3  Primer programa de Python: Calculadora básica  ( First python program basic calculator ) 
Quiero comenzar a aprender Python por diversión y, por lo tanto, podría profundizar en los proyectos de aprendizaje de la máquina más adelante en el futuro ta...




© 2022 respuesta.top Reservados todos los derechos. Centro de preguntas y respuestas reservados todos los derechos