Serial Port Communication This is an efficiency question to the following code, written in MS Visual in C#. If any explanation is missing, please let me know and I will add. What this part of the program does is:

Button is pushed
Motor starts moving (by sending 9-byte array with command)
Waiting for Sensor to register a change in value
Motor stops
Motorposition is saved (command to get position is send and 9-byte array response is send by the motor), to have access to it later on and return there

Calls like rotate() and stop() are commands that write data into a 9-byte-array and write bytearray to serial port.

Now, my main issue is Step 5. I read on Stack Overflow that "if you have to use wait() and there is no point of using the DataReceived implementation". So should I really get rid of this or is there a nice way to handle this? If I do it without the wait() it saves the answer that is send after the stop command (every command also sends an answer).

And would it be more efficient to use DataReceived in step 3? In the beginning I save a value as new 0, and compare the current sensor readout here to this 0 value.

private void button13_Click(object sender, EventArgs e)
{
    //rotate right speed
    string Test = textBox4.Text;
    byte[] A = new byte[9];
    rotate(A, Test);
    //read sensor
    serialPort2.WriteLine("SR,00,002\r\n");
    string b = serialPort2.ReadLine();
    decimal caliber = decimal.Parse(Regex.Split(b, "SR,00,002,")[1]);
    decimal b1 = 0;
    do
    {
        serialPort2.WriteLine("SR,00,002\r\n");
        string z = serialPort2.ReadLine();
        b1 = decimal.Parse(Regex.Split(z, "SR,00,002,")[1]);
    }
    while (b1 <= caliber);
    // stop motor
    byte[] B = new byte[9];
    stop(B);
    //save position
    System.Threading.Thread.Sleep(5000);
    byte[] C = new byte[9];
    getPosi(C);
    System.Threading.Thread.Sleep(5000);
    M = buf;
    //move to 0
    byte[] D = new byte[9];
    MoveTo0(D);
}

In case it is needed for understanding the code above:

//global variables
byte[] buf = new byte[9];
byte[] M = new byte[9];
private void button1_Click(object sender, EventArgs e)      //Ports öffnen
{
    serialPort3 = new SerialPort();
    serialPort3.PortName = "COM6";
    serialPort3.Handshake = Handshake.RequestToSend;
    serialPort3.ReceivedBytesThreshold = 8;
    serialPort3.DataReceived += new SerialDataReceivedEventHandler(DataRecievedHandler);
    serialPort3.Open();
}
private void DataRecievedHandler(object sender, SerialDataReceivedEventArgs e)
{
    SerialPort sp = (SerialPort)sender;
    int bytes = serialPort3.BytesToRead;
    byte[] buffer = new byte[bytes];
    sp.Read(buffer, 0, bytes);
    string hex = BitConverter.ToString(buffer);
    hex.Replace("-", "");
    MessageBox.Show(hex);
    buf = buffer;
}

Other methods:

private void rotate(byte[] A, string Test) //Rotate Motor Right
    {
        A = MB.TMCL_RMR(Test);
        serialPort3.Write(A, 0, A.Length);
    }

    private void stop(byte[] B) // Stop Motor
    {
        B = MB.TMCL_MST();
        serialPort3.Write(B, 0, B.Length);
    }

    private void getPosi(byte[] C) //Get Position
    {
        C = MB.GET_POSI();
        serialPort3.Write(C, 0, C.Length);
    }

    private void MoveTo0(byte[] D) //Move to 0
    {
        D = MB.MoveTo0();
        serialPort3.Write(D, 0, D.Length);
    }

And as example a method from my seperate motorcontrolclass on how the bytearray is created. Each hex-byte references a certain information, last byte is a checksum. If this array is send to the serial port it will request the motorposition as an answer.

public byte[] GET_POSI()
    {
        byte[] E = new byte[9];
        E[0] = 0x1;
        E[1] = 0x6;
        E[2] = 0x1;
        E[3] = 0x0;
        E[4] = 0x0;
        E[5] = 0x0;
        E[6] = 0x0;
        E[7] = 0x0;
        E[8] = 0x08;
        return E;
    }

The code is working as I want it to for several input tests. I just thought there might be a better way than the Thread.Sleep().

I guess you thought about avoiding of Thread.Sleep() method, not Wait() you mentioned in title. I can't test your code because I don't have either COM port nor motor. I would like to give you some general tips how to improve your code.
Let's implement some SOLID and DRY principles. I would start with class Motor which will represent your device. You can implement for example singleton class for this purpose. This helps you to separate your motor logic from other logic such is user interface. I can also imagine your port settings as a part of application configuration.

Motor.cs

public sealed class Motor : IDisposable
{
    public static readonly Motor Instance = new Motor();

    private Motor()
    {
        MotorPort = new SerialPort();
        MotorPort.PortName = "COM6";
        MotorPort.Handshake = Handshake.RequestToSend;
        MotorPort.ReceivedBytesThreshold = 8;
        MotorPort.DataReceived += new SerialDataReceivedEventHandler(dataRecievedHandler);
        MotorPort.Open();
    }

    public SerialPort MotorPort { get; private set; }
    public decimal Position { get; private set; }
    public void StopRotate()    { }
    public void StartRotate(string speed) { }
    public void MoveToPosition(int position) { }
    private void dataRecievedHandler(object sender, SerialDataReceivedEventArgs e)
    {
    }
    public void Dispose()
    {
        if (MotorPort.IsOpen)
        {
            MotorPort.Close();
        }
    }
}

Instead of using global fields (M, buf), encapsulate them into object and set the proper access modifier or better create properties. Use meaningful names your fields and properties. Use Capitalization Convention.

You can isolate your repeating strings into separate static class to make your string constant repository which would be more easily maintainable against direct using of particular string.

StringConstants.cs

static class StringConstants
{
    public const string Prefix = "SR,00,002,";
    public const string PrefixCrLf = "SR,00,002\r\n";
}

Extension methods can simplify your code and make it more readable.

Extensions.cs

static class Extensions
{
    public static decimal GetDecimalOutput(this SerialPort port)
    {
        return decimal.Parse(Regex.Split(port.ReadLine(), StringConstants.Prefix)[1]);
    }
    public static void SendCommand(this SerialPort port)
    {
        port.WriteLine(StringConstants.PrefixCrLf);
    }
}

If you have implemented Motor.cs methods, you can use it or extend it.

Form1.cs

private void btnTest_Click(object sender, EventArgs e)
{
    Motor.Instance.StartRotate(speed);
    Thread.Sleep(5000);
    Motor.Instance.StopRotate();
    Motor.Instance.MoveToPosition(0);

    //read sensor
    serialPort2.SendCommand();
    decimal caliber = serialPort2.GetDecimalOutput();

    do
    {
        serialPort2.SendCommand();
        actualValue = serialPort2.GetDecimalOutput();
    }
    while (actualValue <= caliber);
}

I am not experienced in serial port programming, but I am pretty sure that event SerialPort.DataReceived should be used to invoke subscribed method as you do with your serialPort3. I don't know exactly why you are using Thread.Sleep(), because I cannot see getPosi(C); method.


Converting Arabic numerals to Roman numerals These classes are part of a program I am working on to convert Arabic numerals to Roman numerals and vice-versa.  Do these classes look correct for representing objects in Java?

public class ArabicNumeral {
private final int value;

public ArabicNumeral(final int value) {
    if (value < 0) {
        throw new NumberFormatException("Numbers must be positive");
    }
    this.value = value;
}

public int getThousands() {
    return getCardinalValueFor(1000);
}

public int getHundreds() {
    return getCardinalValueFor(100);
}

public int getTens() {
    return getCardinalValueFor(10);
}

public int getOnes() {
    return getCardinalValueFor(1);
}

private int getCardinalValueFor(final int divisor) {
    return value / divisor % 10;
}
}

public enum RomanNumerals {
I("I", 1),
V("V", 5),
X("X", 10),
L("L", 50),
C("C", 100),
D("D", 500),
M("M", 1000);

private final String symbol;
private final int value;

RomanNumerals(final String symbol, final int value) {
    this.symbol = symbol;
    this.value = value;
}

String symbol() {
    return symbol;
}

int value() {
    return value;
}

public RomanNumerals getMultipleOfFive() {
    return this.ordinal() < RomanNumerals.values().length - 1
            ? RomanNumerals.values()[this.ordinal() + 1] : null;
}

public RomanNumerals getMultipleOfTen() {
    return this.ordinal() < RomanNumerals.values().length - 1
            ? RomanNumerals.values()[this.ordinal() + 2] : null;
}
}

I wouldn't do an ArabicNumeral, too, as this is the default numeric system of java.

From a design view, I would see an object RomanNumeral, which is an aggregation of RomanLiteralobjects, this RomanLiteral, are the constant roman string->decimal value pairs, which can't be computed.

Both classes can extend java.lang.Number

Class RomanLiteral represents one base value of the roman numeral system, it is a String - Integer relation, e.g. "V" -> 5. toString() is overriden to return the String value.

Class RomanNumeral
Represents a fully roman numeral, it is an aggregate of RomanLiterals, which are basic String - Integer relation, e.g. "V" -> 5, and so on.

Attributes

defaultRomanLiteralSet this Set containts the basic constant RomanLiterals
literals this is the List which contains all RomanLiterals that aggregate this RomanNumeral

Methods

RomanNumeral parseRomanNumeral(Integer) is an analogy to Integer#parseInt(String) this method may be static, and returns the RomanNumeral of the Integer param.
add(RomanNumeral) adds an RomanNumeral to the current value of the object and returns it.
add(RomanLiteral) adds the RomanLiteral to the current value of the object and returns it.
toString() is overriden to return the right String representation of the roman numeral - there happens some calculations with the matching values in the default-set. Here actually gets the roman String calculated out of the integer value of the aggregate.

I think that would be a good useable design. I would stay quite near to the Number style of java, and would do my implementation and design in this direction. (analgous methods and so on..)


Implement selection from array by 'random' or 'if/elif conditionals' I'm working on some code examples in 'Automate the boring stuff in python...' to prepare for my first year of CS. I want to know if its better to use an array with random selection below or rather multiple if/elif conditionals to generate output. Any and all suggestions are welcome.

import sys
import random
import time

answer = ['It is certain', 'It is so', 'Try later', 'Ask Again', 'Not good', 'Doubtful']
r = random.randint(0,5)

def getAnswer():
    print (answer[r])

def Main():
    try = input('Try your luck. Yes or no.\n')
    if try == 'yes':
        getAnswer()
        sys.exit()
    else:
        print('May luck be on your side.')
        sys.exit()

if __name__ == '__main__':
    Main()

The array is clean and easy to read, and it takes up much less space than an army of if/elif/else statements. In my opinion, what you have is much better than the alternative.

That said, there are a couple of things that could be changed.

First, either move the random selection into the getAnswer() function, or change the function to accept an argument. What if you decide later on that you want to get multiple answers? Even if you only call getAnswer() once, making one of these changes will make the function's job clearer.

Second, remove the hard-coded array length in the random.randint() arguments. If you later add or remove answers, the arguments to randint() need to be changed, too. This can be avoided by either using random.choice() (ideal if getAnswer() doesn't take any arguments) or by using random.randint(0, len(answer) - 1) or random.randrange(len(answer)).

randrange() is typically preferred over randint(). Everything else in Python treats ranges as including the start index and excluding the end index, but randint() includes the end index. Not only does that make randint() inconsistent with everything else, but it also makes working with lists and sequences more error-prone (notice how clean and neat randrange(len(my_list)) is compared to randint(0, len(my_list) - 1)?).

Here is how these changes might look (along with one or two other changes, unrelated to how the random selection is made).

Option 1: Random selection inside of getAnswer():

import sys
import random
import time

ANSWERS = ['It is certain', 'It is so', 'Try later', 'Ask Again', 'Not good', 'Doubtful']

def getAnswer():
    print (random.choice(ANSWERS))

def Main():
    prompt_string = 'Try your luck. Yes or no.\n'
    while input(prompt_string).strip().lower() in ('yes', 'y')
        prompt_string = 'Try again (yes or no)?\n'
        getAnswer()

    print('May luck be on your side.')
    sys.exit()

if __name__ == '__main__':
    Main()

Option 2: getAnswer() takes an argument

import sys
import random
import time

ANSWERS = ['It is certain', 'It is so', 'Try later', 'Ask Again', 'Not good', 'Doubtful']

def getAnswer(chosen_index):
    print (ANSWERS[chosen_index])

def Main():
    prompt_string = 'Try your luck. Yes or no.\n'
    while input(prompt_string).strip().lower() in ('yes', 'y')
        prompt_string = 'Try again (yes or no)?\n'
        getAnswer(random.randrange(len(ANSWERS)))

    print('May luck be on your side.')
    sys.exit()

if __name__ == '__main__':
    Main()


Rails integration testing: Should we use strings or named routes in http requests? When integration testing Rails applications HTTP request helpers can take a string or a named route as the first argument.

I find myself writing:

test "user visits edit page" do
  homer = users(:homer)
  get "users/#{homer.id}/edit"
  assert_response :success
end

But I can also write:

test "user visits edit page" do
  homer = users(:homer)
  get edit_user_path(homer)
  assert_response :success
end

So which style should I use? And why is it preferable to the other?

Use the 2nd version - i.e. use the path helper methods, don't use strings. Or use a 3rd one, detailed below.

Ideally, you test your routes separately from the controller (in test/routes//spec/routes/ files). That'll test that "/users/homer/edit" indeed routes to the users#edit action. Provided your using Rails' standard resourceful routes, you don't need further tests. Anything more would be you testing that Rails works, which should be unnecessary. You want to test your app.

Secondly, the reason Rails provides path/url helper methods is that hard-coding paths/urls is a recipe for a headache should the representation of those paths ever change. For instance, a simple resources :users, as: :people in the routes file would break all your hard-coded "/users/..." paths.

Thirdly, you're simply writing homer.id. But path helpers call to_param (which you can override in your models) on the object they're passed. So you're not accurately mimicking the way the path helper methods work.

Basically, if you make a change in routes.rb, you don't want to have to update your controller tests - after all, you didn't necessarily change your controllers! You only want to update your routing tests, if you have any.

Alternatively, (edit: this is for controller tests, specifically) you can use a third version, where you specify action and params separately:

get :edit, { id: homer.to_param }

This way, you side-step both hard-coded URLs, route matching, and path/URL helpers entirely. This is probably the "cleanest" solution.


Get title from database based on member country I asked this question before. However, I realized that I had to edit some of my code after I got good answers. Therefore, I'm asking a new question here.

I have the following database:

Each member may have multiple addresses (1:n) stored in table address. It's possible that one member has multiple addresses within one country.
Further, each member may have  multiple titles (m:n) mapped in table title2memb. Each entry idRank has a name which can be found in title.
I want to get the first N members that have at least one address in the country "US"  and I want to display all their titles.
Here is working code, using two JOINS and two where statements for N=2:
<?php

$db       = new mysqli("localhost","root","","example");
$sql      = "SELECT DISTINCT m.name, m.id FROM member m
              INNER JOIN address a ON m.id = a.idMember AND a.country='US' LIMIT 2";
$membersResult   = $db->query($sql);

while($member = $membersResult->fetch_assoc()){
  $id            = $zeile['id'];

  echo $zeile['name'] . ": ";

  $sub_sql       = "SELECT t.name FROM title t INNER JOIN title2memb tm ON
                    t.id = tm.idRank AND tm.idMember='$id'";
  $sub_result    = $db->query($sub_sql);
  while($rank = $sub_result->fetch_assoc())
  {
    echo $rank['name'] . " ";
  }

  echo "<br>";
}

?>

And as a result I get:

Bernd: Bee
Tom: Bear Dog

Is it really necessary to have here 2 nested while clauses? Or can I get both in one SQL?
Some remarks:

I had to use the DISTINCT keyword in my first SQL, otherwise I would get Bernd twice.
I have avoided using prepared statements and encoding the input to make the code more readable. I know that the here presented version is vulnerable to XSS.

Firstly, for questions like this is would be helpful to provide an SQL fiddle. I mocked one up to test on, it only takes a few minutes but would be much faster for you since you have the table definitions.

  Is it really necessary to have here 2 nested while clauses? Or can I get both in one SQL?

You can absolutely get everything you need in one SQL query without nested queries. In fact, you should. Performing database queries within a loop is never ideal and it's always a better idea to get the data you need (even if slightly more than you need) then filter it down with PHP to avoid having to do multiple queries.

The logic

  I want to get the first N members that have at least one address in the country "US" and I want to display all their titles.

OK - so you're aiming for two things - the member's name and their titles. Your conditions are based on the address table data, so you should start your query there:

SELECT `idMember` FROM `address`
WHERE `country` = 'US'
GROUP BY `idMember`
LIMIT 2

Your at least one address in the country "US" condition is covered by WHERE country = 'US', and the GROUP BY will limit your duplicated rows. If you want to see how many addresses each member has (or filter on it) you can add COUNT(*) to the select clause to see. You will adjust the LIMIT as required.

This gives you the member IDs that you need to retrieve titles for. From here, just query the tables you need and join the results above:

SELECT `member`.`id`, `member`.`name`, `title`.`name`
FROM `title`
INNER JOIN `title2memb` ON `title2memb`.`idRank` = `title`.`id`
INNER JOIN (
  SELECT `idMember` FROM `address`
  WHERE `country` = 'US'
  GROUP BY `idMember`
  LIMIT 2
) AS `us_members`
  ON `us_members`.`idMember` = `title2memb`.`idMember`
INNER JOIN `member` ON `member`.`id` = `us_members`.`idMember`;

Note that I've added the member's ID here because you'll use it to group the result set with PHP, as MySQL as a relational database is not able to return multi-dimensional results (see NoSQL/MongoDB/DynamoDB etc if you want that!).

  I have avoided using prepared statements and encoding the input to make the code more readable. I know that the here presented version is vulnerable to XSS.

Noted - won't comment on that.

To implement, you can do something like this:

$country = 'US';
$limit = 2;

$sql = '
    SELECT `member`.`id`, `member`.`name`, `title`.`name` AS `title_name`
    FROM `title`
    INNER JOIN `title2memb` ON `title2memb`.`idRank` = `title`.`id`
    INNER JOIN (
      SELECT `idMember` FROM `address`
      WHERE `country` = "' . $country . '"
      GROUP BY `idMember`
      LIMIT  ' . $limit . '
    ) AS `us_members`
      ON `us_members`.`idMember` = `title2memb`.`idMember`
    INNER JOIN `member` ON `member`.`id` = `us_members`.`idMember`;
';

$membersResult = $db->query($sql);

// Use PHP to group the results by the member ID
$members = array();
while ($member = $membersResult->fetch_assoc()) {
     // Track the member's name and an array of titles against their ID (which is unique)
     if (!array_key_exists($member['id'], $members)) {
        $members[$member['id']] = array(
            'name' => $member['name'],
            'titles' => array()
        );
     }
     // Add the title
     $members[$member['id']]['titles'][] = $member['title_name'];
}

This will give you a simple structured array like so:

Array
(
    [1] => Array
        (
            [name] => Bernd
            [titles] => Array
                (
                    [0] => Bee
                )
        )
    [3] => Array
        (
            [name] => Tom
            [titles] => Array
                (
                    [0] => Bear
                    [1] => Dog
                )
        )
)

Now, loop that array and output the data how you want to:

foreach ($members as $memberId => $member) {
    echo sprintf('%s: %s<br>', $member['name'], implode(' ', $member['titles']));
}

Example output (from a browser):

Bernd: Bee
Tom: Bear Dog

General formatting

While we're here, you should consider a few best practices:

Use single quotes always, unless you require double quotes
Choose a rule for indentation and stick to it. Four spaces is generally accepted as "the norm" for PHP (see while contents)
Add spaces before and after brackets in control structures (see while)
Space out arguments in function calls (see $db)


Parse Java source code I have a bunch of simple interfaces like this one (pretty enough formed, but not guarantee)

package com.example.sources;

import com.google.gwt.resources.client.ClientBundle;
import com.google.gwt.resources.client.ImageResource;
import com.google.gwt.resources.client.ImageResource.ImageOptions;
import com.google.gwt.resources.client.ImageResource.RepeatStyle;

import com.example.sources.MainSceneResource;
import com.example.sources.Preloadable;

public interface MainSceneResources extends ClientBundle, Preloadable {

    @Source("com/example/sources/css/mainscene.css")
    public MainSceneResource mainSceneCss();

    @Source("com/example/sources/img/panorama.jpg")
    @ImageOptions(repeatStyle = RepeatStyle.None)
    public ImageResource mainSceneBackground();

}

I need to:

remove line with package;
change lines with import (delete the import keyword, whitespaces, semicolon at the end) and add stripped packages with the Factory's method addImport(line);
change Preloadable to Generated
copy other lines without changes with the SourceWriter's method sw.println(line)

Currently my code is like:

BufferedReader reader = new BufferedReader(new InputStreamReader(ris));

String line = null;
try {
    while ((line = reader.readLine()) != null) {
        if (line.contains("package") || line.isEmpty()) {
            continue;
            }
        int index = line.indexOf("import");
        if (index != -1) {
            line = line.substring(index + 6, line.lastIndexOf(";")).trim();
            factory.addImport(line);
            continue;
        }
        if (line.contains("Preloadable")) {
            line = line.replace("Preloadable", "Generated");
        }
        sw.println(line);
    }
} catch (IOException e) {
    throw new UnableToCompleteException();
}

The one more notice is that I can't use SourceWriter method sw.println(line) before Factrory factory.addImport(line);

My most worries are about calling line.contains("pattern") in while-loop. As I know "pattern" will be compiled to Java regex Pattern.class. I'm aware it's not good to compile the same Pattern over and over till the loop ends, but don't know how to do this better.
Also I doubt about this line (to strip import lines)

line = line.substring(index + 6, line.lastIndexOf(";")).trim();

Is there a better way to do this?

Generally what you do is dangerous. More dangerous as you might expect. As soon as you have something odd like a line break in a package or import line, a double semicolon or multiple import statements in a line, the code will break. If your keywords ("package", "import", "Generated") are used somewhere inside the interface, it will break as well (note that these words could appear e.g. in comments).

If you don't want to use a "real" Java parser, there are some things you can do to improve your code: If you don't find a closing semicolon for package or import, add the next line as long as you don't have one - or throw at least a meaningful error. You can also check if you have two semicolons in a package or import line. But I think the most important point is to break your loop in 4 parts in order to make your changes as "local" as possible. The first loop deals with the package, the second with the imports, the third with Generated and the fourth just adds the rest without changing. So your are safe that after a step is done, it can't do any harm to the following code. Another thing you could try is to preprocess the code with some formatter / pretty printer, which could get rid of some quirks.

Before the code gets at least a little bit more secure, I wouldn't care about performance too much. Consider adding some JUnit tests (ask your colleagues for some "mean" input classes - you will be surprised which weird monsters they can imagine - and use the more realistic examples).


Can this coffeescripts method be simplified? I'm new to coffeescripts. Can the getSum method be simplified more? Thanks

MyObject =
  checkCondition: (num) ->
    return true if num is 5

  getSum: (num) ->
    total = 0
    total += i for i in [1..num] when @checkCondition i
    total

I tried removing the last total but coffeescripts compiler goes nut :-/

Any help greatly appreciate.

First, return true if num is 5 is the same as just num is 5.
Then, you can convert the explicit loop into a reduce (you can read about it in Wikipedia or MDN):

MyObject =
  checkCondition: (num) -> num is 5

  getSum: (num) ->
    [0..num].reduce (x,y) =>
        if @checkCondition y then x + y else x

Note => instead of plain -> after (x,y). This is fat arrow, it prevents the capture of @ the inside function.


Reading file contents into a string Option 1 - nice and simple

private void GetFileReady()
{
    private StringBuilder fileContents = new StringBuilder();
    using (var sr = new StreamReader("C:\A big file.txt"))
    {
        fileContents.Append(sr.ReadToEnd());
    }
}

Option 2 - less simple

private void GetFileReady2()
{
   private StringBuilder fileContents = new StringBuilder();
   const int bufferSize = 1024;
   var buffer = new Char[bufferSize];
   var count = bufferSize;

   using (var sr = new StreamReader("C:\A big file.txt"))
   {
       while (count > 0)
       {
           count = sr.Read(buffer, 0, bufferSize);
           fileContents.Append(buffer, 0, count);
       }
   }
}

Would option 2 be better for something esoteric like memory allocation?

Is the goal to get a string containing the content of the file? or to add the content of a file to an existing StringBuilder?

if it's the former then Option 1 can drop the string builder altogether ... or better yet.

string value = File.ReadAllText("C:\A big file.txt");

if it's the latter then you might want to think about using StringBuilder.EnsureCapacity() to avoid the overhead of resizing the buffer more then it needs to.


Abstract tree-map function Exercise 2.31.  Abstract your answer
  to exercise 2.30 to produce a
  procedure tree-map with the property
  that square-tree could be defined as

(define (square-tree tree) (tree-map square tree))

I wrote the following solution:

(define (square x) (* x x))

(define (tree-map f tree)
  (cond ((null? tree) null)
        ((pair? tree)
         (cons (tree-map f (car tree))
               (tree-map f (cdr tree))))
        (else (f tree))))

(define (map-tree-map f tree)
  (map (lambda (subtree)
         (if (pair? subtree)
             (cons (map-tree-map f (car subtree))
                   (map-tree-map f (cdr subtree)))
             (f subtree)))
         tree))

(define (square-tree tree) (tree-map square tree))
(define (map-square-tree tree) (map-tree-map square tree))

(define a (list 1 1 (list (list 2 3) 1 2)))

Can this be improved in any way?

Your direct definition of tree-map looks good.

The version using map may be written more succinctly (with the recursive portion factored out) as follows:

(define (tree-map f tree)
  (define (rec tree)
    (map (lambda (subtree)
           ((if (pair? subtree) rec f) subtree))
         tree))
  (rec tree))


Compiling all Exception messages into a string Below is a recursive function I've written that's responsible for compiling a string consisting of a Exception's message, and also each of that Exception's InnerException messages.

I've tried to condense everything to one line, but am unsure if this is the most efficient way of approaching this problem.

public static string GetExceptionMessages(Exception exception, int msgCount = 1)
{
    return exception != null ? string.Format("{0}: {1}\n{2}", msgCount, exception.Message, GetExceptionMessages(exception.InnerException, ++msgCount)) : string.Empty;
}

And here is code showing the above method in action:

public static void Main(string[] args)
{
    try
    {
        Throw1();
    }
    catch (Exception e)
    {
        string messages = GetExceptionMessages(e);
        Console.WriteLine(messages);
    }
    Console.ReadLine();
}
public static void Throw1()
{
    try
    {
        Throw2();
    }
    catch (Exception e)
    {
        throw new Exception("Throw1", e);
    }
}
public static void Throw2()
{
    throw new Exception("Throw2");
}
public static string GetExceptionMessages(Exception exception, int msgCount = 1)
{
    return exception != null ? string.Format("{0}: {1}\n{2}", msgCount, exception.Message, GetExceptionMessages(exception.InnerException, ++msgCount)) : string.Empty;
}

Readability first

  I've tried to condense everything to one line, but am unsure if this is the most efficient way of approaching this problem.

Your primary goal when writing code is readability and maintainability and not squeezing everything into single lines.

Can you read this?

return exception != null ? string.Format("{0}: {1}\n{2}", msgCount, exception.Message, GetExceptionMessages(exception.InnerException, ++msgCount)) : string.Empty;

I cannot. This line is way too long even on my large monitor.

How about this? A little bit better, but still, the third line is too long even with string interpolation instead of string.Format

return
    exception is null
        ? string.Empty
        : $"{messageCount}: {exception.Message}{Environment.NewLine}{GetExceptionMessages(exception.InnerException, ++messageCount)}";

Separation

We need to change the strategy because your method is doing two things at a time:

it's looking for exceptions and
it's building a string

We need to separate them by implementing an extension that will enumerate exceptions:

public static IEnumerable<Exception> DescendantsAndSelf(this Exception exception)
{
    do
    {
        yield return exception;
        exception = exception.InnerException;
    } while (!(exception is null));
}

Then we rewrite the string building method as an extension of the first one:

public static string StringifyMessages(this IEnumerable<Exception> exceptions)
{
    var messages = exceptions.Select((e, i) => $"{i + 1}: {e.Message}")
    return string.Join(Environment.NewLine, messages);
}

So, finally you can chain them like this:

catch (Exception e)
{
    Console.WriteLine(e.DescendantsAndSelf().StringifyMessages());
}

Advantages

This means more code now but this code is testable, modular and can be reused for other things and extensions.


Database Model for Feedback Module I wish to generate an online feedback system. Requirements are

Admin can enter feedback questions
Admin will select these questions and create a feedback session
One Question can be used in many feedback session
User can select any feedback session and answer/rate
one user can answer same feedback session more than once

My table structure is

dummy data (not all columns)

Questions
---------
QuestionId  Question
1           Question 1
2           Question 2
3           Question 3
4           Question 4
5           Question 5

Feedback
--------
FeedbackId  Questions
2           1,2,3,4,5                   -- Questions column has questionid from Question table

 FeedbackResults
    ---------------
    FeedbackResId   FeedbackId  QuetionID Answers    UserId
    1               2           4          4         1
    2               2           1          3         1
    3               2           5          3         1
    4               2           1          2         2

Please rate this design and provide valuable suggestions. Comma separated values are affordable ?

The design need to be simplified. From your explanation, it seems you only need 3 tables (Questions, Answers, and Feedback) the rest are not needed, since you can JOIN these tables.

  Comma separated values are affordable ?

For your design, no you don't need to, and it's not always a good idea to do it. There is some cases you can do it, and might be benefit you, but these are rare cases. Keep things simple and standard, don't join columns or rows into a single value, unless it's a requirement!

So, if your Feedback.Questions meant to store values of questions in comma separated value, I suggest you change that. just give each question a new feedback id, this way it'll be much more manageable.  And instead of storing the question string, just store the questionId (FK) instead. this way it'll be more appropriate and would process faster.

So I suggest you remodel it to :

#ListStatus
- Id [PK|IDENTITY]
- Description

#ListRate
- Id [PK|IDENTITY]
- Description

#FeedbackQuestions
- Id [PK|IDENTITY]
- Question
- CreatedAt
- UpdatedAt
- Deleted
- StatusId
- UserId

#FeedbackAnswers
- Id [PK|IDENTITY]
- QuestionId
- Answer
- CreatedAt
- UpdatedAt
- Deleted
- UserId

#Feedback
- Id [PK|IDENTITY]
- RateId
- QuestionId
- AnswerId
- CreatedAt
- Deleted
- UserId

The Listxxx tables would store a list of fixed descriptive values. For instance, ListRate would store the rate options such as Excellent, Very Good, Good, Fair, Poor ..etc. When a user rates an answer, the Id of ListRate would be inserted into Feedback table.

Also, since we defined them with a prefix List it would be standardized in the system design, for general use purpose on other tables, and best yet, easier to understand (so developers would know the purpose of these tables just from the table name).

I got confused between UserId & CustomerId, but I used userId so you're working with logged-in users doesn't matter if is it from Admin or some customer using the system, at the end, you need to record the Username that made this changes .

Another thing you might need is to add a category for the questions, and maybe the Feedback as well. This will be very helpful in filtering the results. You don't want to depend on any column that store user input with open text, it'll be a real pain in the neck. So, adding a category to the questions, which will store it's values in another table to have two columns (one for displaying text for user, the other one is the id) will make things easier to work with, and if you see that you can add more filtering options (either supporting sub-category or tags ..etc) go for it.

Sample  :

Questions Table

| Id |   Question |            CreatedAt | UpdatedAt | Deleted | StatusId | UserId |
|----|------------|----------------------|-----------|---------|----------|--------|
|  1 | Question 1 | 2019-12-14T00:00:00Z |    (null) |  (null) |        1 | User 1 |
|  2 | Question 2 | 2019-12-14T20:40:15Z |    (null) |  (null) |        2 | User 2 |
|  3 | Question 3 | 2019-12-14T07:05:01Z |    (null) |  (null) |        3 | User 3 |

Answers Table

| Id | QuestionId |   Answer |            CreatedAt | UpdatedAt | Deleted |  UserId |
|----|------------|----------|----------------------|-----------|---------|---------|
|  1 |          1 | Answer 1 | 2019-07-05T00:00:00Z |    (null) |  (null) | User 50 |
|  2 |          1 | Answer 2 | 2019-07-05T00:00:00Z |    (null) |  (null) | User 50 |
|  3 |          1 | Answer 3 | 2019-07-05T00:00:00Z |    (null) |  (null) | User 70 |
|  4 |          2 | Answer 1 | 2019-07-05T00:00:00Z |    (null) |  (null) | User 70 |

Feedback Table

| Id | RateId | QuestionId | AnswerId |            CreatedAt | UpdatedAt | Deleted |  UserId |
|----|--------|------------|----------|----------------------|-----------|---------|---------|
|  1 |      1 |          1 |        3 | 2019-07-05T00:00:00Z |    (null) |  (null) |   Admin |
|  2 |      2 |          2 |        4 | 2019-07-05T00:00:00Z |    (null) |  (null) | User 23 |
|  3 |      2 |          1 |        2 | 2019-07-05T00:00:00Z |    (null) |  (null) |  User 5 |
|  4 |      5 |          1 |        3 | 2019-07-05T00:00:00Z |    (null) |  (null) | User 23 |

Fiddle Demo


Fourier transformation Could someone be kind enough to review this Fourier transformation, please? I'm especially interested in going about unit testing the code (I don't really know where to begin, beyond applying the fft twice to retrieve the input), and have left test functions blank.

If you do spot a bug (and no doubt there are some), please don't tell me. I'd like to find them myself.

/*
  A simple radix 2 fast fourier transform.
*/

#include <complex>
#include <iostream>
#include <cassert>
#define _USE_MATH_DEFINES
#include <cmath>

typedef std::complex<double> Complex;

void TestSpike(Complex* data, int length){
   assert( 1.0 == data[1]);
}

//Generates a spike in the DC bin, used for testing the fft
void GenerateSpike(Complex* data, int length){
  for(int i = 0 ; i < length ; i++)
    {
      if(1 == i){
        data[i] = 1.0;
      } else {
        data[i] =  0.0;
      }
    }
  TestSpike(data, length);
}

void TestTwiddle(){
}

void TestButterfly(){
}

Complex w(int bin, int length){
  return (cos(2*M_PI*bin/length), sin(2*M_PI*bin/length));
}

void butterfly(Complex* data, int bin, int stepSize, int fftlength){
  Complex x0 = data[bin];
  Complex x1 = data[bin+stepSize];

  data[bin]          = x0 + w(bin, fftlength)*x1;
  data[bin+stepSize] = x0 - w(bin, fftlength)*x1;
}

//Decimation in time
 void FFT(Complex *data, int length){
  for(int bflySize = 2 ; bflySize < length ; bflySize *= 2)
    {
      int numBflys = length/bflySize;
      for(int i = 0 ; i < numBflys ; i++)
        {
          int stepSize = bflySize/2;
            for(int j = 0 ; j < bflySize ; j+= stepSize)
            {
               butterfly(data, (bflySize*i)+j, bflySize/2, length);
            }
        }
    }
}

int Reverse(unsigned int i, int size){
  int reversed = 0;
  while(size > 1){
    reversed = (reversed >> 1) | (i & 1);
    i >>= 1;
    size >>= 1;
  }
  return reversed;
}

int main(int argv, char** argc){

  int length;
  std::cout << "Please input the length of the FFT" << std::endl;
  std::cin >> length;
  std::complex<double> data[length];
  GenerateSpike(data, length);
  FFT(data, length);
  for(int i = 0 ; i < length ; i++)
     {
       int j = Reverse(i,length);
       std::cout << data[j] << std:: endl;
     }
  return 0;
}

Using operator== on doubles isn't usually a good idea, although it may be okay in the TestSpike case as you know you've assigned exactly 1.0 to it previously.

I would write GenerateSpike as follows:

void GenerateSpike(Complex* data, int length) {
  std::fill(data, data+length, 0.0);
  if (length >= 2)
    data[1] = 1.0;
}

By the way, you're not checking that the array is of sufficient length in TestSpike.

stepSize in FFT does not depend on the parameters of the second-outermost loop, so it should be in the outermost loop.  You should also use stepSize instead of bflySize/2 in the call to butterfly.

About testing:  w, butterfly, Reverse, and FFT all seem to be pure functions, so you can test them simply by checking the return value.  You might want to get a framework like GTest or Boost.Test to make them easier to write.


Handling success and failure when retrieving product information I've got an Angular controller where I have two functions that are repeated inside two functions:

(function () {

    'use strict';

    angular
        .module( 'app.purchases.products' )
        .controller( 'ReadProductController', ReadProductController );

    ReadProductController.$inject = [ '$scope', 'ReadProductFactory' ];

    function ReadProductController( $scope, ReadProductFactory ) {

        /* jshint validthis: true */
        var vm = this;

        vm.products          = {};
        vm.product           = {};
        vm.getProductsList   = getProductsList;
        vm.getProductDetails = getProductDetails;

        function getProductsList( columnOrder, sortOrder ) {

            var data = {
                columnOrder: columnOrder,
                sortOrder: sortOrder
            };

            ReadProductFactory.listProducts( data, success, fail );

            //The following are the callbacks funcs but they repeat
            //for the getProductDetails func too, should i set them global?
            function success( products ) {
                vm.products = products;
            }

            function fail( error ) {
                console.log( error );
            }
        }

        function getProductDetails( id ) {

            var data = {
                id: id
            };

            ReadProductFactory.detailProduct( data, success, fail );

            function success( product ) {
                vm.product = product;
            }

            function fail( error ) {
                console.log( error );
            }

        }

    }

})();

The above is the controller of a Product Listing view, i've tried to take rest logic to a factory, what i feel is that my code is not completely DRY because of the callback functions, maybe you'll understand better what i've tried to do with the factory code, here it is:

(function () {

    'use strict';

    angular
        .module( 'app.purchases.products' )
        .factory( 'ReadProductFactory', ReadProductFactory );

    ReadProductFactory.$inject = [ 'Restangular' ];

    function ReadProductFactory( Restangular ) {

        return {
            detailProduct: detailProduct,
            listProducts: listProducts
        };

        function detailProduct( data, success, fail ) {

            Restangular
                .one( '/purchases/products/', data.id )
                .all( '/detail' )
                .getList()
                .then( success, fail );

        }

        function listProducts( data, success, fail ) {

            Restangular
                .all( '/purchases/products/list' )
                .getList()
                .then( success, fail );

        }

    }

})();

Somes have told me to use promises instead of callback, how could i do that?
Also, is it good practice to declare empty arrays as I've done?

First, your controller is written weirdly. It could be simplified into:

// Since we're inlining everything, we don't need to wrap it in IIFE.
// Also, `readProductController` isn't a good name. Controllers are things,
// nouns, not verbs.
angular.module( 'app.purchases.products' ).controller('ProductController',[

  // A controller accepts either a function with implicit dependencies based on
  // argument names, or an array of dependency names, and whose last element
  // is the controller function.
  '$scope', 'productFactory'
, function(

  // I've read somewhere that factories and services should be named with first
  // letter as small since we're not exporting a constructor. Reserve Capital
  // names for things that return a constructor function.
  $scope, productFactory
){

  // Not sure why `vm` exactly. It doesn't tell me what it is. Name it something
  // meaningful, like `controller`.
  var controller = this;
  controller.products = {};
  controller.product = {};

  // You can simply inline the functions. No need for the indirection.
  this.getProductsList = function(columnOrder, sortOrder){

    // Promises are simply just objects that hold state for an async operation.
    // They still accept "callbacks" via the `then` method they provide.
    productFactory.getProducts({
      columnOrder: columnOrder,
      sortOrder: sortOrder
    }).then(function(products){
      controller.products = products;
    }, function(error){
      console.log(error);
    });
  };

  this.getProductDetails = function(id){
    productFactory.getProductDetails({
      id: id
    }).then(function(product){
      controller.product = product;
    }, function(error){
      console.log(error);
    });
  };

}]);

We can do the same with your service

// `readProductFactory` isn't really a good factory name. Factories are things
// that create things. They're not actions, so drop the read. It's just
// productFactory starting now.
angular.module('app.purchases.products').factory('productFactory', [
  'Restangular',
function(
  Restangular
){
  return {

    // Functions are "actions", verbs, commands. They should be named in a
    // "do something to that something" manner, like `getList`, or `sendMail`
    getProducts: function(data){

      // Restangular appears to already return a promise (because you "then'ed")
      // All we have to do is just return that promise to the controller so that
      // it listens with `then`.
      return Restangular.all( '/purchases/products/list' ).getList()
    },
    getProductDetails: function(data){

      // Doing the same
      return Restangular.one( '/purchases/products/', data.id ).all( '/detail' ).getList()

    }
  };
}]);


Add-to-cart method has too many conditions I have a method that seems to be too much for one method. I might be validating programming methodology. I just don't know what to do with it. Would a switch statement be better? Should I break it up and provide other methods?

public void addToCart(OrderlineType orderline) throws Exception
{

        if (orderline == null)
        {
            throw new OutOfStockException("Null object pass addToCart");
        }

        if(orderline.getProduct().isRecallStatus())
        {
             throw new ProductRecallException("Product Id: " + orderline.getProduct().getProductId()
                     + " is being Recalled");
        }

        int tempOrderlineQuantity = orderline.getQuantity();
        int tempOrderlineProductId = orderline.getProduct().getProductId();

        if( Inventory.getInstance().getQuantity(tempOrderlineProductId)
                        == tempOrderlineQuantity  )
          {
                   shoppingCart.add(orderline);
                   log.warn("Product " + orderline.getProduct().getProductName()
                           +" ID: "+ tempOrderlineProductId
                           + "after purchase will have no more in stock");

          }
        else if(Inventory.getInstance().getQuantity(tempOrderlineProductId)
                        > tempOrderlineQuantity)
          {
                    shoppingCart.add(orderline);
          }
        else if(Inventory.getInstance().getQuantity(tempOrderlineProductId)
                            == -1)

          {
                        throw new OutOfStockException("PrductId: " + tempOrderlineProductId
                                + " is not listed in the Inventory: "
                                + Inventory.getInstance().getQuantity(tempOrderlineProductId));

           }
         else
          {
                        throw new OutOfStockException("PrductId: " + tempOrderlineProductId
                                                    + " does not have enough to complete this order, the in Stock amount is: "
                                                    + Inventory.getInstance().getQuantity(tempOrderlineProductId));
          }
}

public void addToCart(OrderlineType orderline) throws Exception
{

        if (orderline == null)
        {
            throw new OutOfStockException("Null object pass addToCart");
        }

You almost certainly don't want to treat a null orderline the same as an OutOfStockException. You probably don't need to check this at all. If a null orderline is illegal, just let the function throw the NullPtrException that results.

        if(orderline.getProduct().isRecallStatus())
        {
             throw new ProductRecallException("Product Id: " + orderline.getProduct().getProductId()
                     + " is being Recalled");
        }

This isn't really the best place for rules about whether you can sell a product. Instead, I'd suggest:

        orderline.getProduct().verifyAllowedToSell();

Which will throw the exception if you aren't allowed to sell the product because its recalled, or any other reason.

        int tempOrderlineQuantity = orderline.getQuantity();
        int tempOrderlineProductId = orderline.getProduct().getProductId();

Avoid storing variables in temporary locals unless you really need to.

        if( Inventory.getInstance().getQuantity(tempOrderlineProductId)
                        == tempOrderlineQuantity  )
          {
                   shoppingCart.add(orderline);
                   log.warn("Product " + orderline.getProduct().getProductName()
                           +" ID: "+ tempOrderlineProductId
                           + "after purchase will have no more in stock");

          }
        else if(Inventory.getInstance().getQuantity(tempOrderlineProductId)
                        > tempOrderlineQuantity)
          {
                    shoppingCart.add(orderline);

Pull this out of the if blocks.

          }
        else if(Inventory.getInstance().getQuantity(tempOrderlineProductId)
                            == -1)

          {
                        throw new OutOfStockException("PrductId: " + tempOrderlineProductId
                                + " is not listed in the Inventory: "
                                + Inventory.getInstance().getQuantity(tempOrderlineProductId));

           }
         else
          {
                        throw new OutOfStockException("PrductId: " + tempOrderlineProductId
                                                    + " does not have enough to complete this order, the in Stock amount is: "
                                                    + Inventory.getInstance().getQuantity(tempOrderlineProductId));
          }

Move this entire if block structure into

   Inventory.getInstance().takeQuantity(orderline.getProduct(), orderline.getQuantity());

}

Basically, I'd end up with

public void addToCar(OrderlineType orderline) throws Exception
{
     orderline.getProduct().verifyIsAllowedToSell();
     Inventory.getInstance().takeQuantity(orderline.getProduct(), orderline.getQuantity());
     shoppingCart.add(orderline);
}


Border Radius Preview Tool I am running through an app idea to help improve my knowledge of Javascript. The code attached performs the following:

Performs a regex on the input field to check it is value 0-9 If not,
it clears the field and shows a warning (Although this only seems to work on FF)
If it passes, updates the styles of the preview box on keyup
Adds the styles to a text area that the user can copy when selecting the
button

I am looking for suggestions where I can improve in terms of writing more efficient code. Or pointers where I have possibly used the wrong method to perform a task. One area where I struggled was getting the value of the input field.

This is the first stage for this application as in the near future I plan to add in the ability modify each corner.

Feedback is welcomed

"use strict"

const masterField = document.getElementById('masterField');
const box = document.getElementById('box');
const warn = document.getElementById('warn');
const styleSheet = document.getElementById('styleSheet');

const WARNING_TIME = 2000;
var warningTimer;

masterField.addEventListener('keyup', updateCss);

function updateCss() {
    let inputVal = this.value;
    console.log(this.value);
    const expCase = /[0-9]/;

    if (expCase.test(inputVal)) {
        box.style.borderRadius = inputVal + 'px';
        styleSheet.textContent = 'border-radius: ' + inputVal + 'px';
    } else {
        this.value = '';
        showWarning();
    }
}

function hidewarning() {
    warn.classList.add('hide-warning');
}

function showWarning() {
    clearTimeout(warningTimer);
    warningTimer = setTimeout(hidewarning, WARNING_TIME);
    warn.classList.remove('hide-warning');
}

function copyStyles() {
    styleSheet.select();
    document.execCommand('copy');
}
#box {
            width: 300px;
            height: 175px;
            background-color: red;
        }

        #warn {
            display: block;
        }

        #warn.hide-warning {
            display: none;
        }

        All Corners
        <input id="masterField" class="radius-field" type="number">
        Only numbers 0 - 9 are allowed

    <button onclick="copyStyles()">Copy Styles</button>

    <textarea id="styleSheet"></textarea>

UI

Instead of a number input, a range input could be used - with the minimum value being zero and the maximum value being half of the lesser of the height and width - in this case 88 pixels (i.e. 175px / 2). With such an input there is no need to validate user input to ensure non-numeric characters are entered.

With the current interface, the event handler that fires when a key is pressed does not execute when the value changes from other means (e.g. mouse click). To be thorough, it might be better to observe the change event on the input as well.

Code

The JavaScript code makes good use of const for values that don't get re-assigned, and uses getElementById() (as I mentioned in a review of your Nav scroll code). I like how the constant for the warning time is in all capitals, and all lines are terminated with a semi-colon.

The regular expression could be simplified slightly to use the shorthand character class \d instead of [0-9].


Dynamic building of URLs I'm teaching myself Python and wanted to share this problem as my current solution feels inelegant but I'm not sure of the best way to refactor. If this sort of question isn't appropriate, leave a comment and I'll remove it.

I'm writing a generic library to handle web API requests. The aim is to have a generic core that can be fed with URL templates and parameters - so each new API is just a wrapper rather than duplicating core logic like paging, parsing, etc.

Trouble is, each URL has a completely different structure:

Facebook feeds have an ID variable in the URL endpoint - /{id}/feed/
Facebook insights require per-request parameters as well eg /{id}/insights/page_fans?since={timestamp}&until={timestamp}
Instagram user search requires the ID to be a query parameter - /users/search?q={}

Then all URLs need additional query parameters like access_token which is static.

I had started with a simple string.format() to replace the variables, but this doesn't work with dynamic parameters.

New model is to build the URL from a mixture of endpoint, static parameters and dynamic parameters. I'm then using itertools.product to work out the permutations and then encode the URLs.

Current thinking is to create a set of methods for each common problem (date ranges, etc) and just call each one as required during the setup wrapper.

Here's an example of a working wrapper - can you see any more Pythonic ways to handle this situation?

def insights_impressions_unique(ids, access_token, date_start=None,
                                date_end=None, period="day"):
    if period not in ["day", "week", "days_28"]:
        raise ValueError("Period must be 'day','week' or 'days_28'")

    # Build basic URLs
    _api_domain = "https://graph.facebook.com"
    _api_endpoint = "/v2.2/{}/insights/page_impressions_unique"
    url_template = "{}{}".format(_api_domain, _api_endpoint)

    _formatted_urls = [url_template.format(_id) for _id in ids]

    # Move on to paramaters
    _base_params = [{"access_token": access_token,
                    "period": period}, ]

    # Build date periods
    _utcnow = datetime.datetime.utcnow().replace(tzinfo=pytz.utc)
    if date_end is None:
        date_end = _utcnow
    if date_start is None:
        date_start = _utcnow - datetime.datetime.timedelta(days=-7)
    _date_windows = utils.api_call_time_windows(date_start, date_end, freq=7)
    _date_params = [{'since': x[0], 'until': x[1]} for x in _date_windows]

    _combined_params = product(_base_params, _date_params)

    list_of_params = list()
    for i in _combined_params:
        params = {}
        for d in i:
            for k, v in d.iteritems():
                params[k] = v
        list_of_params.append(urllib.urlencode(params))

    _components = product(_formatted_urls, list_of_params)
    _finalised_urls = ["{0}?{1}".format(_u, _p) for _u, _p in _components]

    manager = RequestManager(urls=_finalised_urls, pagination=False)
    return manager

Full code on this dev branch on GitHub.

By and large, I think this approach makes sense and has a fine grasp of Python idiom. Nonetheless, if I'm going to go straight CR on this (maybe this should be at https://codereview.stackexchange.com/), I think there is a short list of things that're a little funky.

Local variables don't need to be underscore prefixed -- since they're closed over by the function and can't be published, there's no need to mark them as private. The underscore prefix is normally only used for module-level or class-level names, or, occasionally, for a name that would otherwise shadow a built-in (for example, _id or _list -- but really, you can just rename stuff in that situation).

Your construction of list_of_params (which should be declared with a literal [], btw) adds a couple of extra lines when it should just use dict.update.

list_of_params = []
for combined_param_product in combined_param_products:
    params = {}
    for param_dict in combined_param_product:
        params.update(param_dict)
    list_of_params.add(urllib.urlencode(params))

pytz.UTC.localize is preferable to datetime.replace, although it makes no difference in this situation.

This is totally inconsequential, but it bothers me when I see people doing a if value in CONSTANT_LIST. I just see the O(n) time over and over in my head, and it could just be O(1) if two characters were changed. Of no real importance, but could probably be {"day", "week", "days_28"}.

Oh, and don't raise ValueError -- raise a subclass of it. Your client code will love you forever.

Lastly, this isn't Python-specific, but it seems like there's some factoring out to do. Pretty much any one of these wrappers is going to construct a variable number of param option lists and some urls. I propose:

def _finalize_urls(base_urls, *param_options_lists):
    """
    :type base_urls: list[str]
    :type param_options_lists: tuple[list[dict[str]]]
    :rtype: list[str]
    """
    pass

This function would contain pretty much all of your code after your declaration of date_params, and be invoked by wrappers like so:

urls = finalize_urls(base_urls, base_params, date_params)

Actually, I'd probably even make it a static constructor of RequestManager

return RequestManager.for_urls_and_params(base_urls, base_params, date_params)

Anyway, by and large totally solid.


Dynamically assign values I'm looking to improve the efficiency of my code.  Although my current method works, I feel it can be improved.

if ouroraddrlen == (4,):
        ouropip = struct.unpack(">bbbb", payload[6:10]) # Need to change this to accept ipv6 as well
        print "Our I.P : ", ouropip
        notheirip = struct.unpack(">b", payload[10])
        print "No. of their I.P's : ", notheirip
        theiroripv = struct.unpack(">b", payload[11])
        print "Their I.P version:: ", theiroripv
        theiroraddrlen = struct.unpack(">b", payload[12])
        print "Length of their Ip : ", theiroraddrlen
        theirfirstip = struct.unpack(">bbbb", payload[13:17])
        print "First Ip : ", theirfirstip
        theirsecondip = struct.unpack(">bbbb", payload[18:22])
        print "Second Ip : ", theirsecondip

The output is:

Time :  (1401734263,)
Our onion address :
Ip version :  (4,)
Ip length :  (4,)
Our I.P :  ( )
No. of their I.P's :  (2,)
Their I.P version::  (4,)
Length of their Ip :  (4,)
First Ip :  ( )
Second Ip :  ( )

I have removed the real IPs, but they are just IPV4 addresses.

However, what I am wondering, is if it is possible to include an if statement after this section of code:

notheirip = struct.unpack(">b", payload[10])
        print "No. of their I.P's : ", notheirip

where if the notheirip is greater than zero and depending on the length of:

        theiroraddrlen = struct.unpack(">b", payload[12])
        print "Length of their Ip : ", theiroraddrlen

which would be either 4 or 16, then it would set the payload values of the next section.

For example, if notheirip = (2,) and theiroraddrlen = (4,), then I would want it to print out

theirip = struct.unpack(">b       << the number of b required so either 4 or 16 and then the range, this will always start at 13 and go up to either 4 or 16 in the future and loop until all the IPs are displayed.

Your question was a little confusing. But I will try and summarize it before I go into my solution:

You have a struct that you are unpacking and during that process you get the number of IPs 'they' have and the length of those IPs (either 4 or 16)
You want to get all of their IPs dynamically based on that length and number of IPs.

Based on my hopefully correct summary above, you can do this simply with a for loop:

num_their_ips = struct.unpack(">b", payload[10])
len_their_ips = struct.unpack(">b", payload[12])

byte_string = '>{}'.format('b'*len_their_ips[0])
their_ips = []
for count in range(num_their_ips[0]):
    start = 13 + (count*len_their_ips[0])
    end = start + len_their_ips[0]

    ip = struct.unpack(byte_string, payload[start:end])
    print 'IP #{}: {}'.format(count+1, ip)

    their_ips.append(ip)

Now a quick note on your naming conventions. Pythonic convention is to use_underscores in variable names. This is especially needed in your code because your variable names are quite hard to distinguish, as was evident in how much 'trouble' I had converting your names below:

our_op_ip, no_their_ip, their_or_ip_v, their_or_addr_len, their_first_ip, their_second_ip

I would also suggest making your names more descriptive. Right now they feel wordy and are a little confusing. Take no_their_ips as an example: does that mean they don't have any IPs, or is it a shortening of 'number'?

Now, I may be being a tad too purposefully dense. However, this is the way people could look at your code; they may not know what some of the acronyms and abbreviations mean. Writing code with descriptive variable names helps improve the readability of your program.


mvc example: form post I've tried to improve my code: separate HTML/PHP, much clear, next time change will be easier.  After doing research on MVC/OOP, I've made the below code to learn.

I understand this is not the MVC pattern now.  Can anybody help me fix it to become actual MVC?

get the result from db (list all)
get the result from db (list search)

index.php

require_once 'grant.php';
require_once 'controller.php';
$controller = new controller();
$controller -> temp_index();

controller.php

require_once 'model.php';
class controller{
    public $model;
    public function __construct(){
        $this -> model = new model();
    }

    public function temp_index(){
        require 'temp_index.php';
        if($_REQUEST['submit'] == 'get_all'){
            $result = $this -> model -> get_all();
            require 'get_all.php';
        }
        else if($_REQUEST['submit'] == 'get_search'){
            $result = $this -> model -> get_search();
            require 'get_search.php';
        }
    }
}

model.php // please ignore not use PDO

class model{
    public function get_all(){
        $sql = "select * from tb order by id desc";
            $query = mysql_query($sql) or die(mysql_error());
            $result = array();
    while($list = mysql_fetch_array($query)){
                $result[] = $list;
            }
            return $result;
    }
    public function get_search(){
        $search = mysql_real_escape_string($_POST['search']);
        $search = trim($search);
        if($search !== ''){
            $sql = "select * from tb where ac_email like '%$search%'";
            $query = mysql_query($sql) or die(mysql_error());
            if(mysql_num_rows($query) > 0){
                $result = array();
                while($list = mysql_fetch_array($query)){
                    $result['result'][] = $list;
                }
                return $result;
            }
            else{
                $result['statu'] = 'can\'t find search';
                return $result;
            }
        }else{
            $result['statu'] = 'can\'t find search plz input text';
            return $result;
        }
    }
}

get_search.php // view

<?php foreach ($result['result'] as $list) : ?>

<?php print"$list[ac_id]";?>

<?php print"$list[ac_email]";?>

<?php endforeach; ?>

<?php print"$result[statu]";?>

temp_index.php // view

<form action="" method="POST">
    <input type="submit" name="submit" value="get_all">
</form>
<form action="" method="POST">
    <input type="text" name="search">
    <input type="submit" name="submit" value="get_search">
</form>

Object Oriented Programming Improvements

The first thing you can do to improve code like this is to use dependency injection, instead of instantiating objects with new inside of objects.

// Setup your class hierarchies like this.

abstract class Controller
{
    protected $model;

    protected function __construct(Model $model)
    {
        $this->model = $model;
    }
}

class RootController extends Controller // or HomeController extends Controller
{
    public function __construct(Model $model)
    {
        parent::__construct($model);
    }
}

Then you execute:

$controller = new RootController(new RootModel());  // But, read on.

Notice the use of type hinting in the __construct function. Program to an interface, not an implementation.

Use abstract classes (which you cannot make an instance of) to factor out common properties and methods into one place. Extend abstract classes with specific species of classes. This is where the power of polymorphism via the Strategy Pattern comes in to play!

By type hinting with abstract classes, or interface types, you will not have to change your code simply because a different species of child object need to be used at some other point in time.

If you find that the parents of several classes should have the same functionality or constants, factor again and define a PHP Interface and slap it on to a class like this.

class RootController extends Controller implements Printable
{
    public function __construct(Model $model)
    {
        parent::_construct($model);
    }
}

Where in this case, Printable would be a set of virtual functions, and or constants, that form a contract, forcing any class that implements Printable to define certain public functions / hooks so that it can be used as a "Printable" object by unrelated code in other places.

Model-View-Controller Improvements

In terms of MVC, you may want to consider how a Model is going to send data to a View. Using a Controller's method as the working area for passing dat from a Model to a View is common. If a Controller's method can act as a working space for a particular command, then "M" and "V" (Model and View) can use "C's" method as a place to coordinate things.

Given the following, you could use your programming knowledge to devise a solution where a View is passed data from a Model, without injecting an instance of said Model into a View. In other words, you do not want to do this.

abstract class View
{
    protected $model;

    protected function __construct(Model $model)
    {
        $this->model = $model;
    }
}

class NewsletterView extends View
{
    public function __construct(Model $model)
    {
        parent::__constrcut($model);
    }
}

Summarizing, there are two well known options:

Use a method of a Controller (i.e. a command). Have the Model pass a data structure (array or iterable object) to the View.
Give the View a reference to the Model and delegate the chore of getting data to the Model within the View. However, as noted this is not what you want to do.

Note that option #2 can create strong coupling between a Model and a View, because the View must then use methods off of the Model instance to get at data. If a Model's methods change (such as the names, or method parameters), the View will have to change, accordingly.

What you want to do is have a setup like this.

abstract class Controller
{
    protected $model;
    protected $view;

    protected function __construct(Model $model, View $view)
    {
        $this->model = $model;
        $this->view = $view;
    }
}

class NewsletterController extends Controller // or HomeController extends Controller
{
    public function __construct(Model $model, View $view)
    {
        parent::__construct($model, $view);
    }

    /**
    * A totally made up method.
    */
    public function register(array $suscriberData)
    {
        $viewData = $this->model->addSubscriber($suscriberData));
        $this->view=>updateRegistrationPage($viewData);
    }
}

Then you execute:

$controller = new NewsletterController(new NewsletterModel(), new NewsletterView());

(Note: A dependency injection container can resolve class dependencies off screen and just issue you the object you request! Very handy, but not mandatory.)

Your URL to such a command would be:

http://yourdomain.com/newsletter/register

Where "newsletter" is your Controller, and "register" is the command.

Typically, the machinery of a framework (Front Controller / Router / Dispatcher), or just good programming generally, handles transforming the path component of a URL (REQUEST_URI) into an instance of a Controller. In that case, because instances are being created dynamically, you will want to use a class autoloader (as in PHP-FIG PSR-4) at minimum. Using an autoloader can eliminate all but one require statement for accessing classes. You will need one require statement to bring your autoloader code into scope. :-)

This is why people are encouraged to use namespaces with their OO PHP code. Using a PSR-4 compliant autoloader will automatically find and load your custom classes! The implication being, that your include_path no longer needs to list every dog-on path that you have classes loading from! ;-)

However, if you use modular HTML templates for your web pages (require header.php, require footer.php, require aside.php, etc ..), be sure to have the location of those files in your include_path!

Dependency injection containers are not mandatory, but they do simplify the usage of objects by decoupling where a class is instantiated from where it is used. In other words, a DI container is a very precise gumball machine for objects and other data. :-)


Finding the distance between the two closest points in a 2-D plane Here is my code:

import sys
import math
def dist(a,b):
    return math.hypot((b[0]-a[0]),(b[1]-a[1]))

def minimum_distance(x, y):
    points = list(zip(x,y))
    points_y = sorted(points, key= lambda z:z[1])
    points.sort(key=lambda g:g[0])
    return min_dist(points,points_y)

def min_dist(points,points_y):
    if len(points)==2: return dist(points[0],points[1])
    elif len(points)==3: return min(dist(points[0],points[1]),dist(points[0],points[2]),dist(points[1],points[2]))
    ave = (len(points)+1)//2
    yleft = [t for t in points_y if t[0]<=points[ave-1][0]]
    yright = [q for q in points_y if q[0]>=points[ave][0]]
    d1 = min_dist(points[0:ave],yleft)
    d2 = min_dist(points[ave:len(points)],yright)
    d = min(d1,d2)
    arr_split = [point for point in points_y if abs(point[0]- points[ave][0]) <= d]
    d_=2*(10**18)
    for i in range(len(arr_split)-1):
        for j in range(i+1,min(len(arr_split),i+7)):
            temp = dist(arr_split[i],arr_split[j])
            if temp<d_:d_=temp
    return min(d,d_)

Example input:([2,5,1],[3,6,2]) and output is \$ \sqrt 2 \$ since (2,3) and (1,2) are the closest points. The input arrays to minimum_distance are the x and y values of each point respectively and the inputs to min_dist are the points sorted based on their x values and y values respectively.

I have implemented pre-sort and have used computational geometric theory (comparison between 7 points only in the strip) to make the time complexity \$ O(n \log n) \$ and my code works swiftly and correctly for most test cases. However, for some hidden test cases generated by Coursera, my code is taking more than 20 seconds. Any optimisations/ corrections?

Coding style

Your code is difficult to read because it is written very condensed.

There is a well-established coding style for Python, the
PEP8 coding style,
and conformance to that style can be checked online at
PEP8 online.

In your case it reports “missing space around operator” in almost every
code line, also “line too long” and “multiple statements on one line”
violations. As an example,

elif len(points)==3: return min(dist(points[0],points[1]),dist(points[0],points[2]),dist(points[1],points[2]))

is better written as

elif len(points) == 3:
    return min(dist(points[0], points[1]),
               dist(points[0], points[2]),
               dist(points[1], points[2]))

Variable naming

Another aspect which makes the code difficult to understand is the
naming of variables. Here are some examples:

points_y = sorted(points, key= lambda z:z[1])
points.sort(key=lambda g:g[0])

The parameter names z and g seem to be arbitrary. Why are they different
at all if they both refer to a point? Similarly at

yleft = [t for t in points_y if t[0]<=points[ave-1][0]]
yright = [q for q in points_y if q[0]>=points[ave][0]]

Why t and q? I'd suggest point or p in all those places.

def min_dist(points,points_y):

Both parameters contain all points, just sorted differently (by x and y,
respectively). Why the asymmetry in the parameter names? I'd suggest

def min_dist(points_x, points_y):

here, plus a doc comment explaining the meaning of the parameters.

ave = (len(points)+1)//2

makes one think of “average” but is just half of the list length.

Some simplifications

import sys

is not needed. The inner parentheses in

return math.hypot((b[0]-a[0]),(b[1]-a[1]))

are not needed. The elif in

elif len(points)==3:

can be replaced by an if. The slicing in

d1 = min_dist(points[0:ave],yleft)
d2 = min_dist(points[ave:len(points)],yright)

can be simplified to

d1 = min_dist(points[:ave], yleft)
d2 = min_dist(points[ave:], yright)

Validate the input

The program aborts with RecursionError or IndexError if zero or
one point are passed to the function. You should validate the input
and throw an appropriate exception in those cases.

A possible problem

If points[ave-1][0] == points[ave][0] (which means that more than one point
lies on the dividing vertical line) then elements from the point_y
list are assigned to both yleft and yright:

yleft = [t for t in points_y if t[0]<=points[ave-1][0]]
yright = [q for q in points_y if q[0]>=points[ave][0]]

which means that in the recursive calls

d1 = min_dist(points[0:ave],yleft)
d2 = min_dist(points[ave:len(points)],yright)

the second list is not necessarily a rearrangement of the first list anymore.

I haven't found an example where this leads to a wrong result, but it could
be an performance problem because “too large” lists are passed down the
recursion. As an extremal case, if all points have the same x-coordinate
then yleft and yright will always be the complete initial points_y
list.

Performance improvements

points[ave][0]] is accessed multiple times, it might be advantageous to
assign that value to a variable once.

The initial value

d_=2*(10**18)

is a bit arbitrary, but actually not needed: In the following loop we are
only interested in points having a distance less than the previously
computed minimum distance in the left and right half:

d = min(d1, d2)
# ...
for i in range(len(arr_split) - 1):
    for j in range(i+1, min(len(arr_split), i + 7)):
        temp = dist(arr_split[i], arr_split[j])
        if temp < d:
            d = temp
return d

And while it is true that pairs of points having a distance less than d
are at most seven (or six?) indices apart, it seems to be more efficient
to leave the inner loop if the y-coordinates differ by d or more:

for i in range(len(arr_split) - 1):
    for j in range(i + 1, len(arr_split)):
        if arr_split[j][1] - arr_split[i][1] >= d:
            break
        temp = dist(arr_split[i], arr_split[j])
        if temp < d:
            d = temp
return d

Further suggestions

Instead of storing the point coordinates a a tuple you could define a
custom class

class Point:
    __slots__ = ('x', 'y')

    def __init__(self, x, y):
        self.x = x
        self.y = y

so that you can access the x- and y-coordinates of a point with p.x
and p.y instead of subscripting.


Number to Roman Numerals in Python I recently started learning python.

Requirements

Input: Integer between 1 and 3999
Output: String with Roman numerals

I would appreciate it if someone could have look at the code and give me any feedback. I am particularly interested in the Python specific improvements, as I mainly programmed in C before.

Edit: Any feedback on the algorithm is appreciated too.

import sys

symbols1 =     [(1,   "I"), (5,   "V"), (10,   "X")]
symbols10 =    [(10,  "X"), (50,  "L"), (100,  "C")]
symbols100 =   [(100, "C"), (500, "D"), (1000, "M")]

symList = [symbols1, symbols10, symbols100]

def digitToRomanSym (digit, symList):
    """ Converts a single digit into its roman symbols """
    one =  symList[0][1]
    five = symList[1][1]
    ten =  symList[2][1]

    switcher = {
        1: one,
        2: one + one,
        3: one + one + one,
        4: one + five,
        5: five,
        6: five + one,
        7: five + one + one,
        8: five + one + one + one,
        9: one + ten,
    }
    return switcher.get(digit, "")

def getRomanNumerals (n):
    """ Converts a number between 1 and 3999 into Roman numerals """
    maxNumber = symList[-1][-1][0] * 4 - 1 # A symbol can be repeated max 3 times e.g. MMM for 3000

    if (n > maxNumber):
        return "Number too big"

    if (n == 0):
        return "Number too small"

    i = 0
    romanStr = ""
    while (n > 0 and i < len(symList)):
        romanStr = digitToRomanSym(n % 10, symList[i]) + romanStr
        n /= 10
        i += 1

    if (n > 0):
        biggestSymbol = symList[-1][-1][1]
        romanStr = (n * biggestSymbol) + romanStr;

    return romanStr

if (len(sys.argv) == 2):
    number = int(sys.argv[1])
    print(getRomanNumerals(number))
else:
    print("Wrong number of arguments")

Welcome to Code Review!

Most of my suggestions are stylistic, but still something to care about in the Python world.  (PEP8 Style Guide)  I did catch a fairly major bug since you are only accepting 1 - 3999 as valid numbers, so we fix that too.

Redundant Parentheses

Unlike in C or C++ (or even Java to some level) you don't have to wrap conditionals in parentheses typically.  There are some cases where you would need to, but your if conditions are not complex enough to need that.

BUG: If an integer less than 0 is passed, there is no warning about being too small a number

Therefore, if a negative number is passed in at runtime, it outputs nothing and no errors.  We can fix this in one line of code - we just change your == check to a <= check (less than or equal to):

if num <= 0:
    return "Number too small"

Descriptive Parameters

Normally, you should name an parameter for a function in a more descriptive way.  While n can be assumed to be a number, it would be better to have num instead of n as an parameter name for getRomanNumerals to better describe what that is.

My two cents, of course, but in more complex functions where n could lose meaning, you'd be better off with a more descriptive parameter name so you don't lose track of what n does.

Rogue Semicolon!

On line 47, you have a rogue trailing semicolon.  While this doesn't affect running of your program, such semicolons are not necessary.

Style: Function names should be lowercase; multiword names separated with underscores

From the PEP8 (linked above):

  Function names should be lowercase, with words separated by underscores as necessary to improve readability.

  mixedCase is allowed only in contexts where that's already the prevailing style (e.g. threading.py), to retain backwards compatibility.

Therefore, your function names should be renamed as follows:

digitToRomanSym becomes digit_to_roman_sym
getRomanNumerals becomes get_roman_numerals

Style: Parameter variable names usually are lowercase

While not explicitly defined in PEP8 as far as I can tell, most of the time, arguments within a function (for example, symList in digitToRomanSym in your code) tend to be lowercase and if multiword, are underscore-separated.

Style: Local variable names should be lowercase, multiword separated with underscores

Just like function definitions, local variables should be lowercase.  That is, variables within the scope of a single function.

Your code after my changes:

(Filename: int2roman.py)

import sys

symbols1 =     [(1,   "I"), (5,   "V"), (10,   "X")]
symbols10 =    [(10,  "X"), (50,  "L"), (100,  "C")]
symbols100 =   [(100, "C"), (500, "D"), (1000, "M")]

symList = [symbols1, symbols10, symbols100]

def digit_to_roman_sym (digit, sym_list):
    """ Converts a single digit into its roman symbols """
    one =  sym_list[0][1]
    five = sym_list[1][1]
    ten =  sym_list[2][1]

    switcher = {
        1: one,
        2: one + one,
        3: one + one + one,
        4: one + five,
        5: five,
        6: five + one,
        7: five + one + one,
        8: five + one + one + one,
        9: one + ten,
    }
    return switcher.get(digit, "")

def get_roman_numerals(num):
    """ Converts a num between 1 and 3999 into Roman numerals """
    max_number = symList[-1][-1][0] * 4 - 1 # A symbol can be repeated max 3 times e.g. MMM for 3000

    if num > max_number:
        return "Number too big"

    if num <= 0:
        return "Number too small"

    i = 0
    roman_str = ""
    while num > 0 and i < len(symList):
        roman_str = digit_to_roman_sym(num % 10, symList[i]) + roman_str
        num /= 10
        i += 1

    if num > 0:
        biggest_symbol = symList[-1][-1][1]
        roman_str = (num * biggest_symbol) + roman_str

    return roman_str

if len(sys.argv) == 2:
    number = int(sys.argv[1])
    print(get_roman_numerals(number))
else:
    print("Wrong number of arguments")


Total number of stops for an elevator to serve certain people I have seen this question asked around a bit, such as this Code Review question in Java:

  Write an optimal program that would determine the total number of stops
  a elevator has taken to serve X number of people. There is a elevator
  in a building with M floors. This elevator can take a max of X people
  at a time or max of total weight Y. Given that a set of people has
  arrived and their weight and the floor they need to stop given how
  many stops has the elevator taken to serve all the people. Consider
  elevator serves in the first come first serve basis. (N people in Queue)

I am making this code review request to see if I can get any feedback on my C++ implementation.

const int max_number_of_ppl = 3; // X
const float max_weight = 250.0;  // Y
const int number_of_floors = 5; // M

// assume person array is 0,1,....N-1 -> so N people
// people are in a queue , find total number of stops
int solution(const int &N,const std::vector<int> &weight,const std::vector<int> &floor)
{
    int idx = 0;
    float last_weight = 0.0;
    int last_count = 0;
    std::set<int> stops;
    int stop_count = 0;
    while(idx < N)
    {
        //lift full, dont put any more ppl
        if(last_count + 1 > max_number_of_ppl || last_weight + weight[idx] > max_weight)
        {
            stop_count += static_cast<int>(stops.size());
            stops.clear();
            last_weight = 0.0;
            last_count = 0;

        }

        //fill lift with the person
        stops.insert(floor.at(idx));
        last_count++;
        last_weight += weight.at(idx);

        //fetch next person
        idx++;
    }

    //handle last lift session
    if(stops.size() >0)
    {
        stop_count += static_cast<int>(stops.size());
    }

    return stop_count;
}

Simple test:

int main()
{
    int N = 6;
    std::vector<int> weight;
    std::vector<int> stops;
    weight.emplace_back(100);
    weight.emplace_back(70);
    weight.emplace_back(60);
    weight.emplace_back(190);
    weight.emplace_back(100);
    weight.emplace_back(10);

    stops.emplace_back(5);
    stops.emplace_back(5);
    stops.emplace_back(5);
    stops.emplace_back(4);
    stops.emplace_back(3);
    stops.emplace_back(3);

    std::cout<<solution(N,weight,stops);

    return 0;
}

First of all I'd like to suggest you use a for-loop, since it would be easier to see, that you're going to increment the loop variable in every iteration independent of any conditions. Then you could also exchange your idx variable for i since the first does not convey more or less semantics than the second and it is somewhat unwritten convention.

Next: Why use a static_cast<int>for incrementing the stop_count? I think you would not need that, if you defined stop_count as a size_t.

Also: Was there some sort of requirement that solution() takes N as an argument? You could get rid of that parameter, since you have the weight-vector, which in turn has a size().

Furthermore: What is the parameter M (number_of_floors) going to be used for? I do not see much use for it other than boundary checking in the program, in a real world scenario no one could press button 10 in a 5-story building anyway.


how to pass a block to a function as a parameter I'd like to refactor the following code:

case @request.path
when '/'
  Rack::Response.new do |response|
    do_stuff
  end
when '/env' then Rack::Response.new(@env.to_html)
end

And extract Rack::Response.new, something like the following:

Rack::Response.new case @request.path
when '/'    then Proc.new {|response| do_stuff }
when '/env' then @env.to_html
end

But I get

  stringable or iterable required

What's the correct implementation (Ruby 1.9.2)?

In order to pass a block to a function, you need to prefix it with an ampersand.

block_or_string = case @request.path
  when '/'    then Proc.new { |response| do_stuff }
  when '/env' then @end.to_html
end

response = if block_or_string.is_a?(Proc)
  Rack::Response.new(&block_or_string)
else
  Rack::Response.new(block_or_string)
end

This is pretty ugly. It might be an improvement to wrap the strings in simple procs.

block = case @request.path
  when '/'    then Proc.new { |response| do_stuff }
  when '/env' then Proc.new { @end.to_html }
end

Rack::Response.new(&block)


Memory-safe file-reading in C I am trying to write a single function that will read a certain number of bytes from a C file, or the entire file, based on the arguments. I want it to be memory-safe of course, and deal with NULL terminators and the like. Not only that, but it should be efficient.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

char *read(FILE *file, long bytes) {
    /* This function takes a FILE pointer <file> and reads <bytes> bytes from it. These
     * bytes are returned as a char* back to the calling function and should be freed
     * since they were malloc'd. This function was an attempt to recreate the
     * .read( [bytes] ) method on python file objects */

    /**********************************
     Set bytes to 0 to read entire file
     **********************************/

    long fileSize = 0; // File size to zero before byte check

    if (bytes == 0) {
        fseek(file, 0L, SEEK_END); // seek to end
        fileSize = ftell(file); // find size of file by checking end
        rewind(file); // back to the beginning for reading
    } else {
        fileSize = ++bytes; // extra byte for NULL
    }

    char buffer[16]; // We will read the file 16 bytes at a time
    char *file_out = malloc(fileSize + 1);
    if (file_out == NULL) {
         fputs("Could not allocate enough memory", stderr);
         exit(EXIT_FAILURE);
    }

    while (fgets(buffer, 16, file) != NULL) {
        strncat(file_out, buffer, 16);
    }

    return file_out;
}

Is this efficient enough for large files and fringe cases?

Instead of fgets, you can use fread to read directly into the file_out:

int totalRead = 0;
int size;
while( fileSize > total_read && (size = fread(file_out + totalRead, 1, fileSize - totalRead, file) != 0) {
    totalRead+=size;
}
if(totalRead < fileSize){
    //couldn't read entire file
}
file_out[total_read] = '\0';

return file_out;

This avoids the \$O(n^2)\$ complexity of doing strncat on a growing string.

Doing an exit from within a helper function is not recommended because the calling code cannot recover from it. Instead, return a special value that specifies something went wrong.

Also, make up your mind about the naming convention of camelCase vs. snake_case.


Build a pizza and choose your toppings I've created a scenario below, in which a user can build a pizza and choose their toppings, then order their pizza:

require 'active_record'
require 'logger'

ActiveRecord::Base.establish_connection adapter: 'sqlite3', database: ':memory:'
ActiveRecord::Base.logger = Logger.new $stdout
ActiveSupport::LogSubscriber.colorize_logging = false

ActiveRecord::Schema.define do
  self.verbose = false

  create_table :pizzas do |t|
    t.string :name
  end

  create_table :pizzas_toppings_groups do |t|
    t.integer :pizza_id
    t.integer :toppings_group_id
  end

  create_table :toppings_groups do |t|
    t.string :name
    t.integer :user_id
  end

  create_table :toppings do |t|
    t.integer :toppings_group_id
    t.string :name
  end

  create_table :orders do |t|
    t.integer :user_id
  end

  create_table :ordered_pizzas do |t|
    t.integer :order_id
    t.string :name
  end

  create_table :ordered_pizza_toppings do |t|
    t.integer :ordered_pizza_id
    t.integer :topping_id
  end
end

class Pizza < ActiveRecord::Base
  has_many :pizzas_toppings_groups
  has_many :toppings_groups, through: :pizzas_toppings_groups
  has_many :toppings, through: :toppings_groups
end

class PizzasToppingsGroup < ActiveRecord::Base
  belongs_to :pizza
  belongs_to :toppings_group
end

class ToppingsGroup < ActiveRecord::Base
  belongs_to :user
  has_many :pizzas_toppings_groups
  has_many :pizzas, through: :pizzas_toppings_groups
  has_many :toppings

  validates :name, presence: true
end

class Topping < ActiveRecord::Base
  belongs_to :toppings_group

  validates :name, presence: true
end

class Order < ActiveRecord::Base
  belongs_to :user
  has_many :ordered_pizzas
end

class OrderedPizza < ActiveRecord::Base
  belongs_to :order

  has_many :ordered_pizza_toppings
  has_many :toppings, through: :ordered_pizza_toppings
end

class OrderedPizzaTopping < ActiveRecord::Base
  belongs_to :ordered_pizza
  belongs_to :topping
end

# admin defines pizzas and available toppings groups and related toppings

cheesy_pizza = Pizza.create(name: "cheeeeeeeese")

cheap_cheesy_group = cheesy_pizza.toppings_groups.create!(name: 'basic cheeses')
mozza = cheap_cheesy_group.toppings.create!(name: 'mozzarella')

fancy_cheesy_group = cheesy_pizza.toppings_groups.create(name: 'fancy cheeses')
goat = fancy_cheesy_group.toppings.create!(name: 'goat')
cheddar = fancy_cheesy_group.toppings.create!(name: 'cheddar')

# imagine there is a fancy user interface where the user can choose
# toppings from topping groups, but certain groups
# have min/max limits on the toppings you can choose, etc etc
# for now here's a method that
# just takes in chosen toppings, and ties it to the ordered_pizza:
def create_order(pizza:, chosen_toppings:)
  Order.transaction do
    order = Order.create
    ordered_pizza = order.ordered_pizzas.create(name: pizza.name)
    ordered_pizza.toppings << chosen_toppings # imagine this line is lots of complicated validations
    order
  end
end

# user orders a pizza

jims_order = create_order(pizza: cheesy_pizza, chosen_toppings: [mozza, goat])

jims_order.reload

jims_order.ordered_pizzas

# for business reasons, some toppings are no longer available, and are removed.

mozza.destroy
goat.destroy

jims_order.reload
jims_order.ordered_pizzas.first.toppings

Historical integrity is now compromised! The toppings are missing on Jim's ordered pizza and all other ordered pizzas with those toppings.

A few questions:

Is this an ideal way to saves orders with their customized pizzas?
What's the best way to avoid this historical integrity problem?

Saving customized pizzas this way, looks OK but it also depends on how the rest of application is structured. I think your biggest concern here should be the historical data integrity you mentioned in your second question.

When it comes to data, the context they live in, helps you figure out how you should treat them. For example, when running a business like a pizza store with online ordering, toppings once available to customers can't just be "deleted". They can become unavailable, discontinued or something in these terms, but not deleted. You can't respond to customers that ask you to add pepperoni to their pizzas: "Oh, we deleted pepperoni!".

Once data hit production they will always be part of your application, despite how their status changes, so you have to treat them as if they will always be around.

This is somewhat a common issue and there are a few ways to solve it. I can mentioned two of them which I've used in the past.

1st Approach: Soft Data Deletion

The first approach, would be to go with some sort of soft delete policy for your data. You can find many posts across the internet about this technique and you can find many gems for ActiveRecord which could be used right out of the box. Practically, what you achieve with soft deletes, is that, when you're deleting data, you're not really deleting them, but you mark them as deleted so you can exclude them from your queries.

The most common ways to do this is either by adding a new column to your table that indicates a record has been deleted (e.g. deleted_at:datetime, deleted:boolean), or by adding a separate, mirror table in which "deleted" data will be moved (e.g. deleted_toppings). Both options have their pros and cons so it really depends on how the application is designed.

Keep in mind that soft delete may add complexity to your app. For example what will happen when you require Topping#name to be unique and you try to add a new topping with an identical name that already belongs to a soft-deleted topping?

2nd Approach: Separate model for customized ordered pizzas

The second approach would be to treat OrderedPizzaToppings in a completely different way. The idea here is to decouple OrderedPizzaTopping from PizzaTopping.

It could look like this:

class OrderedPizza < ActiveRecord::Base
  belongs_to :order

  has_many :ordered_pizza_toppings
end

class OrderedPizzaTopping < ActiveRecord::Base
  belongs_to :ordered_pizza
end

Every time an ordered pizza is created you will dump each Topping's data to a OrderedPizzaTopping instance and you'll save it. For example you could have something like this:

class OrderedPizzaTopping < ActiveRecord::Base
  belongs_to :ordered_pizza

  def self.from(topping = Topping.new)
    create!(data: topping.to_h)
  end
end

# ....

def create_order(pizza:, chosen_toppings:)
  Order.transaction do
    order = Order.create
    ordered_pizza = order.ordered_pizzas.create(name: pizza.name)
    ordered_pizza.toppings << chosen_toppings.map { |t| OrderPizzaTopping.from(t) }
    order
  end
end

If you noticed the only thing that OrderedPizzaTopping does is to store a hash version of PizzaTopping. Just a simple data structure containing name, description, price, image url, etc. Now, even if the PizzaTopping gets hard-deleted at some point, you'll always have the raw data which will allow you to represent the Order in any way you want. Optionally, you could store a reference to the original PizzaTopping in case it exists in your database.

Historical data integrity is not about deleted data only. What will happen if someone changes the price for a Topping that has been around for a year? All orders that include pizzas containing this topping will be affected.  I think the keyword here is auditing. Auditing a model, means to log data changes throughout time.


Iterative “post order” tree traversal I wanted to get feedback on the correctness and efficiency of this post order traversal routine that my wife and I find really easy to grasp.

def postOrderTraversal(root: TreeNode):
  curr = root
  stack = []

  while stack or curr:
    if curr is None:
      if stack[-1].right is None or popped == stack[-1].right:
        popped = stack.pop()
        print(popped.val)
      else:
        curr = stack[-1].right
    else:
      stack.append(curr)
      curr = curr.left

Congratulations on finding code you enjoy!

This appears to be walking through a binary tree you have defined (or possibly from toytree?).  Walking through this with an explicit stack is one way of thinking of the problem, and the code can be a good learning experience.

An alternate method would be to use the Python's recursion system, where you make code vaguely like:

   def walk(tree):
        if tree is not None:   # ignore walking into an empty node
            walk(tree.left)
            walk(tree.right)
            print(tree.val)

It takes a a little brain stretching to see these as equivalent.  As an exercise you should explore making the function a generator.  That is, would call it as:

  for value in tree.walk():
      print(value)

To do this, you need to replace a print with a yield, and a recursive call with a yield from.   Try it: it does a different stretch on the brain.

As far as efficiency, you need to use the the timeit module to see what does run fast.   In general, tree structures with a single value do poorly for really large numbers because memory access is a bit slow.  However, profiling is necessary for optimizing by less than factors of n (the size of the tree).  Orders of magnitude can be judged by eye and logic while unexpected internals might double the speed of some code.

You might notice that your code will currently fail on being passed an empty tree (root=None), as popped will be checked before it is assigned.  I am not sure why you are checking if popped equals the right node.

Keep hacking!  Keep notes.