Validating a date in a grid This function checks if the EndDate of my grid is valid or not. If it is not valid, it turns the bgcolor of the cell to red color with cell-error class.

I need some help in optimizing this block of code. It looks dirty right now.

function onAccrualEndDateChange(current) {
var endDate = $(current).val();
if (endDate != "") {
    var actualEndDate = endDate;
    if (actualEndDate != null) {
        var tempEndDate = parseInt(actualEndDate.getMonth() + 1) + "/" + actualEndDate.getDate() + "/" + actualEndDate.getFullYear();
        if (ValidateDate(tempEndDate)) {
            var startDate = $(current).closest("tr").find("input[id$='StartDate']").val();
            if (startDate != "") {
                var actualStartDate = startDate;
                if (actualStartDate != null) {
                    var tempStartDate = parseInt(actualStartDate.getMonth() + 1) + "/" + actualStartDate.getDate() + "/" + actualStartDate.getFullYear();
                    if (ValidateDate(tempStartDate)) {
                        if (actualStartDate.getDate() > actualEndDate.getDate() || actualStartDate.getMonth() > actualEndDate.getMonth() || actualStartDate.getFullYear() > actualEndDate.getFullYear()) {
                            $(current).closest("td").addClass("cell-error");
                        } else {
                            $(current).closest("td").removeClass("cell-error");
                        }
                    }
                    else {
                        $(current).closest("td").addClass("cell-error");
                    }
                }
                else {
                    $(current).closest("td").addClass("cell-error");
                }
            } else {
                $(current).closest("tr").find("input[id$='StartDate']").closest("td").addClass("cell-error");
            }
        }
        else {
            $(current).closest("td").addClass("cell-error");
        }
    }
    else {
        $(current).closest("td").addClass("cell-error");
    }
} else {
    $(current).closest("td").removeClass("cell-error");
}
}

This is the code to validate the date:

function ValidateDate(date) {
//This validates date with leap year in mm/dd/yyyy or mm/d/yyyy or m/dd/yyyy or m/d/yyyy date format.
var regex = new RegExp("^(((0?[1-9]|1[012])/(0?[1-9]|1\\d|2[0-8])|(0?[13456789]|1[012])/(29|30)|(0?[13578]|1[02])/31)/(19|[2-9]\\d)\\d{2}|0?2/29/((19|[2-9]\\d)(0[48]|[2468][048]|[13579][26])|(([2468][048]|[3579][26])00)))$");

return (date.match(regex));
}

This isn't "optimized", I just tried to make it more readable.
As I cant test it (really wish people would make jsbin examples to test with, buggered if Im going to do it) I have no idea if it works ;)

function onAccrualEndDateChange(current) {
  var endDate = $(current).val();

  if (endDate === "") {
    $(current).closest("td").removeClass("cell-error");
    return;
  }
  if (endDate === null) {
    $(current).closest("td").addClass("cell-error");
    return;
  }

  var actualEndDate = {
    month: endDate.getMonth(),
    date: endDate.getDate(),
    fullYear: endDate.getFullYear()
  };

  var tempEndDate = parseInt(actualEndDate.month + 1, 10) + "/" + actualEndDate.date + "/" + actualEndDate.fullYear;

  if (!ValidateDate(tempEndDate)) {
    $(current).closest("td").addClass("cell-error");
    return;
  }

  var startDate = $(current).closest("tr").find("input[id$='StartDate']").val();

  if (startDate === "") {
    $(current).closest("tr").find("input[id$='StartDate']").closest("td").addClass("cell-error");
    return;
  }
  if (startDate === null) {
    $(current).closest("td").addClass("cell-error");
    return;
  }

  var actualStartDate = {
    month: startDate.getMonth(),
    date: startDate.getDate(),
    fullYear: startDate.getFullYear()
  };

  var tempStartDate = parseInt(actualStartDate.month + 1, 10) + "/" + actualStartDate.date + "/" + actualStartDate.fullYear;

  if (!ValidateDate(tempStartDate)) {
    $(current).closest("td").addClass("cell-error");
    return;
  }

  if (actualStartDate.date > actualEndDate.date || actualStartDate.month > actualEndDate.month || actualStartDate.fullYear > actualEndDate.fullYear) {
    $(current).closest("td").addClass("cell-error");
  } else {
    $(current).closest("td").removeClass("cell-error");
  }
}


Fastest C++ code to get common prefix length In a C++ program, I have the following function to find the length of the longest common prefix between to char arrays a and b. String b is guaranteed to be shorter than string a.

Is there a faster char array comparison code (for example, with some library function)? I wish to optimize this code for speed.

long len_common_prefix(char a[], char b[]) {
    char* p = a;
    char* q = b;
    while ((*p == *q) && (*q)) {
        p++;
        q++;
    };
    return p-a;
}

I've found the following code to be about 10 times faster that the original one I gave.  Is there an even faster alternative?

long long int len_common_prefix(char a[], char b[]) {
    long long int* p = (long long int*)a;
    long long int* q = (long long int*)b;
    int ratio = sizeof(long long int) / sizeof(char);
    while (*p == *q) {
        p++;
        q++;
    }
    long long int long_long_diff = p-(long long int*)a;
    char* p2 = (char*)p;
    char* q2 = (char*)q;
    while (*p2 == *q2) {
        p2++;
        q2++;
    }
    return long_long_diff * ratio + (p2 - (char*)p);
}

The C++ way would be to call std::mismatch and be done with it

#include <algorithm>    // mismatch
#include <cassert>      // assert
#include <cstddef>      // ptrdiff_t
#include <cstring>      // strlen
#include <iostream>     // cout
#include <iterator>     // distance

std::ptrdiff_t len_common_prefix_base(char const a[], char const b[])
{
    assert(std::strlen(b) <= std::strlen(a));
    return std::distance(b, std::mismatch(b, b + std::strlen(b), a).first);
}

If you absolutely have to improve on the base line performance, you can rewrite your second solution as two successive calls to std::mismatch. First, by reinterpreting the char* as long long int* and calling std::mismatch at the block level. Secondly, after finding the first char in the first mismatching block (which could be the final partial block containing the remaining chars) and doing another std::mismatch over the remaining chars.

std::ptrdiff_t len_common_prefix_10x(char const a[], char const b[])
{
    assert(std::strlen(b) <= std::strlen(a));
    using block_type = long long int;

    auto p = reinterpret_cast<block_type const*>(a);
    auto q = reinterpret_cast<block_type const*>(b);

    auto const n = std::strlen(b);
    auto const num_blocks = n / sizeof(block_type);

    auto block_mismatch = std::mismatch(q, q + num_blocks, p);
    auto b2 = reinterpret_cast<char const*>(block_mismatch.first);
    auto a2 = reinterpret_cast<char const*>(block_mismatch.second);

    return std::distance(b, std::mismatch(b2, b + n, a2).first);
}

int main()
{
    char const a[] = "hello world is a trivial exercise";
    char const b[] = "hello world is a trivial example";

    std::cout << len_common_prefix_base(a, b) << '\n';
    std::cout << len_common_prefix_10x(a, b) << '\n';
}

Live Example that prints 27 (the same as your two implementations).

What you are doing with your pointer cast to long int is very fragile, at the very least it's implementation-defined at possibly undefined behavior (for which no warning is required, and that includes the case that your code runs 10x faster on your system but crashes after you turn on compiler optimizations or port it to another system).

Some issues that you have to be very careful about when doing pointer casting:

alignment, if your char[] is part of a struct, you need to be sure it is aligned at 8-byte boundaries to avoid unaligned memory access from your long long int pointers.
endianness, you need to make sure that the bytes within a long long int are in the same order as the char[] array

I'd like to see a reproducible benchmark before being able to comment on whether a 10x improvement is really possible on std::mismatch.


Custom countdown second timer I'm making a card game and I found useful to include a 60 second countdown timer. Here is my approach:

import java.util.Timer;
import java.util.TimerTask;

public class SecondTimer {

    private Timer timer;
    private int countDown;
    private int secondsLeft;

    public SecondTimer() {
        timer = new Timer();
    }

    public void reset() {
        secondsLeft = countDown;
        // Decrease seconds left every 1 second.
        timer.schedule(new TimerTask() {
            @Override
            public void run() {
                secondsLeft--;
                if (secondsLeft == 0) {
                    timer.cancel();
                }
            }
        }, 0, 1000);
    }

    public void setCountDown(int seconds) {
        this.countDown = seconds;
    }

    public int getSecondsLeft() {
        return secondsLeft;
    }
}

Is this considered clean? How can I test such classes?

Avoid using java.util.Timer. See Checking if a Java Timer is cancelled and java.util.Timer: Is it deprecated? Instead use a ScheduledExecutorService.
When changing to a ScheduledExecutorService, don't cancel or shutdown your ScheduledExecutorService, instead cancel the FutureTask that you will get when scheduling a task.
Don't decrease a counter once a second. Nothing guarantees that it will be called with exactly 1000 ms periodicity.

In your reset method, use System.nanoTime() (not System.currentTimeMillis) to calculate when the time is up, such as:

this.targetTime = System.nanoTime() + seconds * 1_000_000_000;

Then calculate in getSecondsLeft, by using System.nanoTime() again, how many seconds remain until the target nanosecond value has been reached.


Model validation from within the Service Layer I'm interested in the idea of not having my BLL dependent on ASP.NET MVC. This means that I would like to have model binding occur within my service layer. But I want to take advantage of ASP.MVC's model validation as well. So I'm considering returning a custom validation dictionary from my BLL service methods like so

    [Authorize(Roles = "ADMIN")]
    [HttpPost]
    public ActionResult Create(PostEditViewModel postEditViewModel)
    {
        BlogPost post = new BlogPost();

        postEditViewModel.MapTo(post);

        //returns custom validation dictionary
        var dic = _postService.CreatePost(post);

        //extension method that allows Controller ModelState to merge with customer error dictionary (returns ModelState)
        if (!ModelState.Merge(dic).IsValid)
            return View();

        return RedirectToAction("Index", "Home");
    }

and merge the returned validation errors with my Controller's ModelState using an extension method on ModelStateDictionary (the extension method will be part of the MVC application and not the BLL). Can anyone foresee any problems going down the road? Is there a more elegant or intelligent way to accomplish what I'm trying to do here? I've also considered this approach.

It feels somewhat awkward to answer a question this old, but there are some things that can be done.

First, I want to comment on the naming, simply from the fact that I'm currently building an ASP.NET app which is very large, and I've learned through the process that the ViewModel name becomes cumbersome, and is partially a lie. I've begun to name my 'View Models' as 'DTOs' instead, that is: Data-Transfer Object.

This has begun to make my code much more clear, as it's now obvious that the DTO can be a two-way object: data can be fed to it from the back-end or from the UI.

Regarding the service-layer handling database access: this is a good approach, I'll tell you that a lot of my code in this project I've mentioned previously is in the controller, and it's cumbersome. What I would change is reject the idea of needing to create a dict (or pass it to the .Merge method) controller-side, and do one of two things:

Create a BaseController that will handle adding the validation errors to the ModelState itself, though this can become complicated.
Pass the Controller to the method doing the validation, and allow it to add the errors to the ModelState.

It's been a while since I've touched ASP.NET MVC 3, but in 4 and 5 you can directly manipulate the ModelStateDictionary, and use .Add or .AddModelError. I do this on various validation methods to remove that responsibility from the controller, and reduce the call to:

[Authorize(Roles = "ADMIN")]
[HttpPost]
public ActionResult Create(PostEditViewModel postEditViewModel)
{
    BlogPost post = new BlogPost();

    postEditViewModel.MapTo(post);
    _postService.CreatePost(post, this);

    if (!ModelState.IsValid)
        return View();

    return RedirectToAction("Index", "Home");
}

Another pointer: I would recommend converting your Roles attribute parameters to constants, I've found that it becomes unpleasant to change the name of a Role later, if you need to. I actually have a Constants.Roles class that has each role string associated to a type, so that if I have to change the name from Controllers to Inventory, it's right there, and my code can still reference the InventoryControllers constant. (You can also use the constants with XML-docs to make things more descriptive in the future.)

You have:

BlogPost post = new BlogPost();

But then:

var dic = _postService.CreatePost(post);

Be consistent. If you're using var in one location, use it in the others. I used to be anti-var period because I felt it made my code look less organized, but to be honest, it helps align all the variable names to the fifth textual column (counting the space), so it's easy to identify where something is declared. (Of course, with the highlighting Visual Studio does it's not necessary, and F12 is a wonderful tool, but it's still helpful at times.)

It also allows you to ignore what type it is when declaring. I'd have to type BlogPost out in one of those two places on that line (though one can argue that with intellisense when you type BlogPost thing = new it automatically suggests BlogPost() to finish it) so I don't mind changing the manual typing from BlogPost post = to var post = new BlogPost();, personally I type faster that way anyway.


Loop cleanly through different classes If I have a number of different classes which manage certain tests. At the moment I have to launch each test individually which results in a lot of if statements.

I wonder if there is a way of putting all of these in a loop to manage the launching of each test and retrieving of results more cleanly.  Does anyone have any advice? Can anyone improve on this? Is this use of classes optimal (I'm not a professional programmer)?

class Engine(object):
    """Class that holds information about the
tests relevant to the engine"""

    def __init__(self):
        self.run = True # Flag to run the test or not
        self.kg = 20.5 # Some information required by this test
        self.ok = 'Not run' # Final result to say if the test was passed

class Wheels(object):
    """Class that holds information about the
tests relevant to the wheels"""

    def __init__(self):
        self.run = True
        self.number = 4
        self.ok = 'Not run'

class Door(object):
    """Class that holds information about the
tests relevant to the doors"""

    def __init__(self):
        self.run = True
        self.colour = 'red'
        self.ok = 'Not run'

class Car(object):
    """Class that holds information about the
different tests available"""

    def __init__(self):
        self.engine = Engine()
        self.wheels = Wheels()
        self.door = Door()

motor = Car()

if motor.engine.on is True:
    import engine_tests # An external file containing everything about engine tests
    motor.engine.ok = engine_tests.launch(weight=motor.engine.kg) # Return success/failure of tests

if motor.wheels.on is True:
    import wheel_tests
    motor.wheels.ok = wheel_tests.launch(nwheels=motor.wheels.number)

if motor.door.on is True:
    import door_tests
    motor.door.ok = door_tests.launch(shading=motor.door.colour)

print 'Tests completed'
print 'Engine: ', motor.engine.ok
print 'Wheels: ', motor.wheels.ok
print 'Door: ', motor.door.ok

The params for testing are different from each object, so I think this part should be put in the classes and present a uniform interface, this is what classes are about.

I suggest 2 versions: either you only put the params needed for test in the classes, or (better in my opinion), you put the entire call to the test in the classes. With some syntactic sugar, this leads to

Only the test data in the classes

class Engine(object):
    """Class that holds information about the
tests relevant to the engine"""

    def __init__(self):
        self.run = True # Flag to run the test or not
        self.kg = 20.5 # Some information required by this test
        self.ok = 'Not run' # Final result to say if the test was passed

    def get_test_params(self):
        return { 'weight': self.kg }

class Wheels(object):
    """Class that holds information about the
tests relevant to the wheels"""

    def __init__(self):
        self.run = True
        self.number = 4
        self.ok = 'Not run'

    def get_test_params(self):
        return { 'nwheels': self.number }

class Door(object):
    """Class that holds information about the
tests relevant to the doors"""

    def __init__(self):
        self.run = True
        self.colour = 'red'
        self.ok = 'Not run'

    def get_test_params(self):
        return { 'shading': self.colour }

class Car(object):
    """Class that holds information about the
different tests available"""

    def __init__(self):
        self.engine = Engine()
        self.wheels = Wheels()
        self.door = Door()

motor = Car()

for partName in 'engine wheels door'.split():
    part = getattr(motor, partName)
    part.on = True # So the test is run (everything is off in your example)
    if part.on: # No need to write "is True", this is what "if" is about
        test_module = __import__(partName + '_tests')
        part.ok = test_module.launch(**part.get_test_params())

print 'Tests completed'
print 'Engine: ', motor.engine.ok
print 'Wheels: ', motor.wheels.ok
print 'Door: ', motor.door.ok

Running the test from the classes

class Engine(object):
    """Class that holds information about the
tests relevant to the engine"""

    def __init__(self):
        self.run = True # Flag to run the test or not
        self.kg = 20.5 # Some information required by this test
        self.ok = 'Not run' # Final result to say if the test was passed

    def run_test(self):
        import engine_tests
        return engine_tests.launch(weight=self.kg)

class Wheels(object):
    """Class that holds information about the
tests relevant to the wheels"""

    def __init__(self):
        self.run = True
        self.number = 4
        self.ok = 'Not run'

    def run_test(self):
        import wheels_tests
        return wheels_tests.launch(nwheels=self.number)

class Door(object):
    """Class that holds information about the
tests relevant to the doors"""

    def __init__(self):
        self.run = True
        self.colour = 'red'
        self.ok = 'Not run'

    def run_test(self):
        import door_tests
        return door_tests.launch(shading=self.colour)

class Car(object):
    """Class that holds information about the
different tests available"""

    def __init__(self):
        self.engine = Engine()
        self.wheels = Wheels()
        self.door = Door()

motor = Car()

for partName in 'engine wheels door'.split():
    part = getattr(motor, partName)
    part.on = True # So the test is run (everything is off in your example)
    if part.on: # No need to write "is True", this is what "if" is about
        part.ok = part.run_test()

print 'Tests completed'
print 'Engine: ', motor.engine.ok
print 'Wheels: ', motor.wheels.ok
print 'Door: ', motor.door.ok

If there's only one car, the last part can become:

for part in (motor.engine, motor.wheels, motor.door):
    part.on = True # So the test is run (everything is off in your example)
    if part.on: # No need to write "is True", this is what "if" is about
        part.ok = part.run_test()


Crawling for emails on websites given by Google API I'm trying to build an app which crawls a website to find the emails that it has and prints them.  I also want to allow the user to type "false" into the console when they want to skip the website (maybe the user has already found 2 emails and doesn't need any more).

Is the way I'm approaching this the best way? If not, then how can I improve, and what am I missing?

require "nokogiri"
require "json"
require 'mechanize'
require 'anemone'
require "typhoeus"
require "timeout"

class String

    def to_bool()
        return true if self == "true"
        return false if self == "false"
        return nil
    end
end

class Query
            def initialize(keyword)
            10.times do |n|
                    num = (n * 10 + 1).to_s
                    p num
                    req = Typhoeus::Request.new("https://www.googleapis.com/customsearch/v1?key=[my_key]&cx=018020274830505137072:utuofm0ugh0&q=" + keyword + "&start=" + num, followlocation: true)
                    res = req.run
                    File.open("file.json","w") do |file|

                    file.write(res.body)
                    end
                    continue = "true"
                    fs = File.read("file.json");
                    string = JSON.parse(fs);

                  string["items"].each do |item|
                      p continue.to_s + "<- item"

  begin
    Anemone.crawl("http://" + item["displayLink"] + "/") do |anemone|
      anemone.on_every_page do |page|
        if continue.chomp.to_bool == false
          raise "no more please"
        end
        request = Typhoeus::Request.new(page.url, followlocation: true)
        response = request.run
        email = /[-0-9a-zA-Z.+_]+@[-0-9a-zA-Z.+_]+\.[a-zA-Z]{2,4}/.match(response.body)
        if email.nil?
        else
          p email
          begin
            continue = Timeout::timeout(2) do
              p "insert now false/nothing"
              gets
            end
          rescue Timeout::Error
            continue = "true"
          end
        end
      end
    end
    rescue Exception => e
      p e
      continue = "true"
      next
    end
    p "---------------------------------------------------------"
end

                    p "new request"

                end

            end
    end

qs = Query.new("texas+web+development")

As @tokland pointed out, the major thing that jumps out when reading your code is the indentation problems. In most Ruby code you see, the standard indentation is 2 spaces. It looks like you are using hard-tabs, which are generally a bad idea. You should explore the settings of your text editor -- most have an option to insert spaces instead of tabs when you press the "tab" key. Here is your code would look like if indented properly:

require "nokogiri"
require "json"
require 'mechanize'
require 'anemone'
require "typhoeus"
require "timeout"

class String
  def to_bool()
    return true if self == "true"
    return false if self == "false"
    return nil
  end
end

class Query
  def initialize(keyword)
    10.times do |n|
      num = (n * 10 + 1).to_s
      p num
      req = Typhoeus::Request.new("https://www.googleapis.com/customsearch/v1?key=[my_key]&cx=018020274830505137072:utuofm0ugh0&q=" + keyword + "&start=" + num, followlocation: true)
      res = req.run
      File.open("file.json","w") do |file|
        file.write(res.body)
      end
      continue = "true"
      fs = File.read("file.json");
      string = JSON.parse(fs);
      string["items"].each do |item|
        p continue.to_s + "<- item"
        begin
          Anemone.crawl("http://" + item["displayLink"] + "/") do |anemone|
            anemone.on_every_page do |page|
              if continue.chomp.to_bool == false
                raise "no more please"
              end
              request = Typhoeus::Request.new(page.url, followlocation: true)
              response = request.run
              email = /[-0-9a-zA-Z.+_]+@[-0-9a-zA-Z.+_]+\.[a-zA-Z]{2,4}/.match(response.body)
              if email.nil?
              else
                p email
                begin
                  continue = Timeout::timeout(2) do
                    p "insert now false/nothing"
                    gets
                  end
                rescue Timeout::Error
                  continue = "true"
                end
              end
            end
          end
        rescue Exception => e
          p e
          continue = "true"
          next
        end
        p "---------------------------------------------------------"
      end
      p "new request"
    end
  end
end

qs = Query.new("texas+web+development")

For your to_bool() function, this might be a little nitpicky, but I would rewrite it as a case statement like this:

class String
  def to_bool()
    case self
      when "true"; true
      when "false"; false
      else; nil
    end
  end
end

The URL you're passing to Typhoeus::Request.new is ratehr long. You might consider doing something like this to shorten your line lengths a little:

base_url = "https://www.googleapis.com/customsearch/v1?key=[my_key]&cx=018020274830505137072:utuofm0ugh0&q="
req_url = base_url + keyword + "&start=" + num
req = Typhoeus::Request.new(req_url, followlocation: true)

Or, if you want to be really diligent about not having long lines, you could do this:

base_url = "https://www.googleapis.com/customsearch/v1"
key = "[my_key]"
cx = "018020274830505137072:utuofm0ugh0"
req_url = "#{base_url}?key=#{key}&cx=#{cx}&q=#{keyword}&start=#{num}"

You have a part of your code that goes if email.nil? (nothing) else (something). A better way to put this would be:

unless email.nil?
  p email
  # etc.

As a general note, you could make your code more concise by using less "intermediate" or "throw-away" variables. Take advantage of Ruby's method chaining and try condensing your code like this:

...

# No need to define a variable res; req.run is short enough
req = Typhoeus::Request.new(req_url, followlocation: true)
File.open("file.json", "w") do |file|
  file.write(req.run.body)
end

...

# You can get rid of the fs and string variables and do this:
JSON.parse(File.read("file.json"))["items"].each do |item|
  p continue.to_s + "<- item"
  # etc.

...

# You could change the name of the variable from request to req,
# for consistency -- you named another Typhoeus request "req"
# earlier in the code, and it doesn't look like you still need
# that variable, so there's no harm in re-using the name "req."

# Notice how you can eliminate the need for the variables "response"
# "email" like this:
req = Typhoeus::Request.new(page.url, followlocation: true)
email_pattern = /[-0-9a-zA-Z.+_]+@[-0-9a-zA-Z.+_]+\.[a-zA-Z]{2,4}/
unless email_pattern.match(req.run.body).nil?
  # etc.

You might consider changing if continue.chomp.to_bool == false to unless continue.chomp.to_bool, or even if continue.chomp == "false". In fact, I think I like the last way the best -- you could totally do away with monkey-patching a String#to_bool method and just compare continue.chomp to "true" or "false". It's your call, of course. :)

Lastly, this is just my 2 cents, but I think you could simplify the way you're using continue. If I'm understanding correctly, it starts as "true" and you want the program to keep running unless the user types "false" when prompted. I would consider doing away with your String#to_bool method and just comparing whether or not continue.chomp.downcase equals "stop", "exit" or "quit". You could do something like this:

stop_words = ["stop", "exit", "quit"]
continue = ""

...

if stop_words.include? continue.chomp.downcase
  raise "no more please"
end

This would save you from having to keep doing continue = "true" to make sure the program doesn't stop. As long as the value of continue.chomp.downcase is not one of the stop words, the program will keep running.

I admit, though, that I don't really understand the use of continue in p continue.to_s + "<- item", so maybe there is something I'm missing.

Anyway, hope this helps!


get city and state using zip code, scrape in Python I'm looking for any and all feedback on quality, style and efficacy of the code. If there's a simple way to get the header put in, I'd love to hear that.

The code is used to take zip codes from a csv file and and spit back out the city and state where available.

import sys
import os
import csv
from bs4 import BeautifulSoup
import requests

def main():
    pass

def getCityState(zipCode):
# given zip code, return city and state if available
    zipCode = str(zipCode)
    url = "http://www.city-data.com/zips/" + zipCode + ".html"
    r = requests.get(url)
    data = r.text
    soup = BeautifulSoup(data)
    if soup.findAll(text="City:") ==[]:
        cityNeeded = soup.findAll(text="Cities:")
        for t in cityNeeded:
            return t.find_next('a').string

    else:
        cityNeeded = soup.findAll(text="City:")
        for t in cityNeeded:
            return t.find_next('a').string

def getTableFromCSV(fileName, settings):
    with open(fileName, settings) as fp:
        reader = csv.DictReader(fp, skipinitialspace=True)
        table = [row for row in reader]
        header = reader.fieldnames
    return header,table # header and table are lists of dicts

def printTable(header, table):
# print table in console
    print"begin printTable"
    for row in table:
        print row
    print"end printTable"

def addToList(theList, position, newItem):
    theList.insert(position, newItem)
    return theList

def addToDict(theDict, newKey, newItem):
    theDict[newKey] = newItem
    return theDict

def checkZips(table):
# check zips and insert the city and town if there is a good zip
    for row in table:
        stringNeed = row['zip']
        if not stringNeed.isdigit(): # check to see if it's a number
            row["cityState"] = "zip not number"
            pass
        else:
            if not len(stringNeed) == 5:  #check to see if it's 5 digits long
                row["cityState"] = "zip not 5 digits"
            else:
                cityState = getCityState(stringNeed)
                row["cityState"] = cityState
                # print row
     return table

if __name__ == '__main__':
    print"begin main"

    # get table from csv file
    header, table = getTableFromCSV("propertyOutputTest.csv","rUbw")

    # add city, state to header
    header.insert(0,"cityState")

    #check zips and insert city and state whree available
    checkZips(table)

    # write table to a csv file that replaces the old csv file
    with open("propertyOutputTest.csv","wb") as fp:
        writer = csv.DictWriter(fp, header)
        for row in table:
            writer.writerow(row)

    print"end main"

First, a few general comments about the 'main function' in Python. The __name__ == '__main__' idiom is a way to distinguish between simply importing a module and launching one. You may want to simply import the module you just wrote to reuse the checkZips function elsewhere, but you don't want the main function to run when doing that.

Some people simply write:

if __name__ == '__main__':
    main()

to run a function called main() when running this module (eg. with python mymodule.py). This is only a convention, the function could be called whatever you want it to be, or you could simply put all the code in the conditional, as you've done here. TextMate provided you with a main() function with a pass statement to specify that it does nothing and avoid a syntax error. Long story short: remove that main function!

import sys
import os
import csv
from bs4 import BeautifulSoup
import requests

Your module lacks a docstring. It could contain the explanation you gave us. Read more about Python code conventions in PEP 8 and docstring conventions PEP 257.

def main():
    pass

Remove that, as said earlier.

def getCityState(zipCode):
# given zip code, return city and state if available

This should be a docstring.

    zipCode = str(zipCode)

Your zip code is already a string, you don't need to convert it first.

    url = "http://www.city-data.com/zips/" + zipCode + ".html"
    r = requests.get(url)
    data = r.text
    soup = BeautifulSoup(data)

You may not need all those variables, but that's up to you (eg. you could simply write BeautifulSoup(r.text)).

    if soup.findAll(text="City:") ==[]:

Be careful about indentation and follow PEP8 closely (eg. using a PEP 8 checker). Here you need a space after the ==, it makes the whole code easier to read and allows everyone to focus on the important matters.

        cityNeeded = soup.findAll(text="Cities:")
        for t in cityNeeded:
            return t.find_next('a').string

    else:
        cityNeeded = soup.findAll(text="City:")
        for t in cityNeeded:
            return t.find_next('a').string

First, you might want to know about list comprehensions, this can be written as return [t.find_next('a').string for t in cityNeeded]. Second, Don't repeat yourself! Having duplicated code increases the likelihood of errors, eg. when you only fix one part. It is easy here, you can simply move the loop out of the conditional. You can also use a regular expression to remove the conditional entirely (untested):

cityNeeded = soup.findAll(text=re.compile("City:|Cities:"))

Make sure to import re.

def getTableFromCSV(fileName, settings):
    with open(fileName, settings) as fp:
        reader = csv.DictReader(fp, skipinitialspace=True)
        table = [row for row in reader]
        header = reader.fieldnames
    return header,table # header and table are lists of dicts

Nothing much to say here, so I will talk about coding style. :) PEP 8 says that you should write get_table_from_csv. If you don't like it, you don't have to, it's just how most Python coders write code. Second, header,table should be header, table. Third, PEP 8 says you need a double space before inline comments (return header, table  # ...). Fourth, this isn't really a comment, and would be a better fit in the docstring. Fifth, about indentation: don't mix tabs and spaces, and no trailing spaces after your lines. (You didn't have any trailing spaces in this function though).

def printTable(header, table):
# print table in console
    print"begin printTable"
    for row in table:
        print row
    print"end printTable"

print is statement in Python 2, but a function in Python 3. I'd advise you to switch to Python 3 since Python 2 will soon be the past of Python, and at least write things such as print("end printTable") to prepare yourself to write Python 3. There are a few major differences and many small differences between the two version, so the earlier you switch, the easier it will be to get acustomed to Python 3.

Also, remove debug prints when you don't need them anymore.

def addToList(theList, position, newItem):
    theList.insert(position, newItem)
    return theList

def addToDict(theDict, newKey, newItem):
    theDict[newKey] = newItem
    return theDict

Come on, you don't need a function for those!

Overall, don't be overwhelmed by my comments. The code is nice, but there a lots of little things to improve. :)


Method that returns description attribute of enum value I have the following method in .NET Core that returns the description attribute of an enum value. I think this code can be more elegant. I'd appreciate some suggestions.

public static string GetDescription(System.Enum input)
{
   Type type = input.GetType();
   MemberInfo[] memInfo = type.GetMember(input.ToString());

   if (memInfo != null && memInfo.Length > 0)
   {
      object[] attrs = (object[])memInfo[0].GetCustomAttributes(typeof(DescriptionAttribute), false);
      if (attrs != null && attrs.Length > 0)
      {
         return ((DescriptionAttribute)attrs[0]).Description;
      }
    }

    return input.ToString();
}

Type type = input.GetType();
   MemberInfo[] memInfo = type.GetMember(input.ToString());

You should use more meaningful variable names like enumType and enumMembers and your code could use some more LINQ and vars.

if (memInfo != null && memInfo.Length > 0)

This null check is unnecessary. The docs says about GetMember:

  An array of MemberInfo objects representing the public members with the specified name, if found; otherwise, an empty array.

So we can remove one null check and test only the description-attribute and turn it into a ternary operator:

public static string GetDescription(System.Enum value)
{
    var enumMember = value.GetType().GetMember(value.ToString()).FirstOrDefault();
    var descriptionAttribute =
        enumMember == null
            ? default(DescriptionAttribute)
            : enumMember.GetCustomAttribute(typeof(DescriptionAttribute)) as DescriptionAttribute;
    return
        descriptionAttribute == null
            ? value.ToString()
            : descriptionAttribute.Description;
}

  I think this code can be more elegant

With C# 6 it can be just a small chain of method calls...

public static string GetDescription(Enum value)
{
    return
        value
            .GetType()
            .GetMember(value.ToString())
            .FirstOrDefault()
            ?.GetCustomAttribute<DescriptionAttribute>()
            ?.Description
        ?? value.ToString();
}


Two permutation iterators for classes in Python with and without a generator I am just testing out a couple of small classes for learning purposes. Here is my current working code:

class Permutation:
     '''An int list a permutation if starting at any index, one can reach every other index by
     repeated index accesses. The list [4, 0, 3, 1, 2] is a permutation: for example index 0
     stores a 4; index 4 stores a 2; index 2 stores a 3; index 3 stores a 1; index 1 stores a 0,
     bringing us back to that starting position after visiting every index.'''
     def __init__(self, p, start):
         self.p = p
         self.start = start

     def __iter__(self):
         class P_iter:
             def __init__(self, p, start):
                 self.p = p
                 self.start = start
                 self.curr = start
                 self.stahp = False

             def __next__(self):
                 if not self.stahp:
                     current = self.curr
                     self.curr = self.p[self.curr]
                     self.stahp = self.curr == self.start
                     return current
                  else:
                      raise StopIteration
         return P_iter(self.p, self.start)

class Permutation2:
     '''similar to the description above, but its __iter__ method should be implemented by a
     generator (not a P_iter nested class)'''
     def __init__(self,p,start):
         self.p = p
         self.start = start

     def __iter__(self):
         curr = self.start
         stahp = False
         while not stahp:
             yield curr
             curr = self.p[curr]
             stahp = curr == self.start
         else:
             raise StopIteration

I'm looking to clean the code a bit and maybe make it simpler. The code is fully working currently and generates the desired output.

Well Permutation has basically equivalent code to Permutation2, but is doing things the hard way.  Therefore, I'll just look at Permutation2, particularly __iter__().

 def __iter__(self):
     curr = self.start
     stahp = False
     while not stahp:
         yield curr
         curr = self.p[curr]
         stahp = curr == self.start
     else:
         raise StopIteration

You shouldn't need to explicitly raise StopIteration.  Iteration automatically stops when the flow of control reaches the end of the function.

I find the LoLcat spelling of "stop" amusing.  However, flag variables are generally a bad idea.  You would be better off writing if curr == self.start: break, for example.  However, I'd just choose to write it like this:

def __iter__(self):
    yield self.start
    curr = self.p[curr]
    while curr != self.start:
        yield curr
        curr = self.p[curr]

(Once again, the lack of a do-while loop in Python makes life difficult.)

Take care to indent your code very carefully in Python, as whitespace is significant.  Four spaces is a very strong norm, established in PEP 8.  You've used five spaces at the first level.


MS VS solution structure w.r.t. dependency injection unit tests? Is the following MS VS solution structure and classes OK w.r.t. easy unit tests with dependency injection? Would your unit tests look similar?

If you call the calculations below over a WPF module, would you skip a test over the WPF since complicated, or would you still conisder testing it over the WPF module ?

Solution

ProjectWork

Classes

IDev.cs

public interface IDev
{
  IToMeasure Measure();
}
public interface IToMeasure
{
  double Value{ get; set; }
}

CalculatingUnit.cs

public class CalculatingUnit
{
   int count = 0,
       maxMeasuredValues = 20;

   public double doMeasures(IDev myDevice)
   {
      // simulating measuring each Xms
      while (count < maxMeasuredValues )
      {
         return myDevice.Measure();
         count++;
      }
   }
}

MyDevice.cs

// non-deterministic (random) values
public class MyDevice: IDev
{
   private readonly MyRandom myRandomVar = new Random(0);

   public IToMeasure Measure()
   {
      var meas = new Measure { Value = myRandomVar.NextRandom(2, 0.5) };
      return meas;
   }
}

TestDevice.cs

// deterministic values
public class TestDevice : IDevice
{
   int nrValues = 6;
   int gIdx = 0,
   gCount = 0;
   int[] testMeasures = { 1, 2, 3, 4, 5, 6 };

   public IToMeasure Measure()
   {
      gIdx = gCount % nrValues;
      var meas = new Measure { Value = testMeasures[gIdx] };
      gCount++;

      return meas;
   }
}

myDeviceWPF.cs // it seems complicated testing over WPF, so I dont test with it, would you also skip it ?

class myDeviceViewModel : INotifyPropertyChanged
{
  ...
}

ProjectTest

myDevice_Test.cs

[TestClass]
public class myDevice_Test
{
    [TestMethod]
    public void Test_doMeasures()
    {
        double actual = 1;
        double expected = 21.068;

        TestDevice myDevice;
        CalculatingUnit myCalculatingUnit;

        myDevice = new TestDevice();
        myCalculatingUnit = new CalculatingUnit();

        actual = myCalculatingUnit.doMeasures(myDevice );
        Assert.AreEqual(expected, actual , 0.001, "Wrong calculations");
    }
}

First, some quality/stylistic comments:

This won't do what you want/isn't legal:

  while (count < maxMeasuredValues )
  {
     return myDevice.Measure();
     count++;
  }

// non-deterministic (random) values
private readonly MyRandom myRandomVar = new Random(0);

Random initialized with a fixed seed is deterministic.  Even with no seed (current time) it is technically deterministic, but that's pickier.  Also, I don't see how you are using MyDevice in the code sample.

    double actual = 1;
    double expected = 21.068;

    TestDevice myDevice;
    CalculatingUnit myCalculatingUnit;

    myDevice = new TestDevice();
    myCalculatingUnit = new CalculatingUnit();

    actual = myCalculatingUnit.doMeasures(myDevice );

In general, prefer to declare and assign a value at the same time.  This will make the functions shorter and more readable.

Other than that, the organization looks fine.


Using Custom Attributes to Populate Members from IDataReader Just learned how to use custom attributes in C# and the first useful thing I did with it was to make a class object representing a database table. The constructor can accept an IDataReader and populate the members based on the custom attribute. Is this a good usage of custom attributes?

[AttributeUsage(AttributeTargets.Property)]
public class DatabaseField: Attribute
{
    public string Name { get; set; }
    public DatabaseField( string name )
    {
        Name = name;
    }
}

public class Employee
{
    [DatabaseField("Employee_ID")]
    public string EmployeeId { get; set; }

    [DatabaseField("LogonId")]
    public string Username { get; set; }

    [DatabaseField("Email")]
    public string EmailAddress { get; set; }

    [DatabaseField("NAME")]
    public string DisplayName { get; set; }

    [DatabaseField("First_Name")]
    public string FirstName { get; set; }

    [DatabaseField("Middle_Name")]
    public string MiddeName { get; set; }

    [DatabaseField("Last_Name")]
    public string LastName { get; set; }

    [DatabaseField("Name_Suffix")]
    public string NameSuffix { get; set; }

    [DatabaseField("Job_Title")]
    public string JobTitle { get; set; }

    [DatabaseField("Department")]
    public string Department { get; set; }

    [DatabaseField("Emp_Type")]
    public string EmployeeType { get; set; }

    [DatabaseField("SupervisorID")]
    public string ManagerId { get; set; }

    public Employee( IDataReader reader )
    {
        foreach( PropertyInfo prop in typeof(Employee).GetProperties() )
        {
            foreach( object attr in prop.GetCustomAttributes(true) )
            {
                DatabaseField field = attr as DatabaseField;
                if( null != field )
                {
                    object value = reader[field.Name];
                    prop.SetValue(this, value == DBNull.Value ? null : value, null);
                }
            }
        }
    }
}

public class DatabaseField: Attribute

According to the naming guidelines, the names of attribute classes should end with Attribute, so this class should be called DatabaseFieldAttribute.

public string Name { get; set; }

I don't see any reason why the setter should be public, so I would make it private.

public Employee( IDataReader reader )

I don't think all this code should be here. Instead, it should be in a library that's not specific to your types.

Have you considered adding the knowledge of your naming conventions to your code, so that not all properties need the attribute?

Also, I would go the other way around: for each column in the reader, find the appropriate property to set. That way, if your type is missing a property, you can learn about it.

foreach( object attr in prop.GetCustomAttributes(true) )

I would use the generic extension method GetCustomAttribute<T>() here, to simplify your code:

var attr = prop.GetCustomAttribute<DatabaseFieldAttribute>(true);

if( null != field )

There is not reason to use Yoda conditions in C#. The issue they're addressing is that buggy code like if (field = null) is valid C and C++. But it's not valid C#.


Adding or replacing a sheet in an Excel file I have the following function and want to simplify the code to make it more readable by eliminating redundant lines:

Function AddSheet(shtName As String, Optional readOnlyFlag As Boolean = True) As Boolean  'Add new worksheet to current workbook at the end

    If readOnlyFlag And IsSheet(shtName) Then
        AddSheet = False
        Exit Function
    ElseIf Not readOnlyFlag Then
        If IsSheet(shtName) Then DeleteSheet (shtName)
        With ThisWorkbook:
            .Sheets.Add(After:=.Sheets(.Sheets.Count)).Name = shtName
        End With
        AddSheet = True
    End If

End Function

Since the first condition (If readOnlyFlag And IsSheet(shtName)) will only assign a return value of False to the function (which should already be the default, as I understand) and then exits the function, would it be acceptable to leave out that aspect of the logic and just have the following:

    If Not readOnlyFlag Then
        If IsSheet(shtName) Then DeleteSheet (shtName)
        With ThisWorkbook:
            .Sheets.Add(After:=.Sheets(.Sheets.Count)).Name = shtName
        End With
        AddSheet = True
    End If

In other words, is it best practice to be explicit like the first code block, by stating that the function returns False if that occurs, or to avoid what might be considered redundancy, by using the simplified second version?

VBA does a lot of implicit things. And it can get confusing, and it can cause surprising bugs.

Your function is implicitly Public. Is it meant to be called from another module? Or is it only called from within the same module it's in? Being explicit about everything makes the intent of the code much easier to understand.

The parameters are implicitly passed ByRef, and yet, their values aren't reassigned in the body of the function (which is good!), so they could, and should, be passed ByVal. It's really a shame that ByRef is the implicit default in VB6/VBA.

The comment that's trailing after the function's signature, should be either immediately above, or immediately below the signature - avoid horizontal scrolling, and don't hide information from maintainers.

The comment could be clearer about why a Boolean return value is returned.

Private Function AddSheet(ByVal shtName As String, Optional ByVal readOnlyFlag As Boolean = True) As Boolean
'Appends a new worksheet to the active workbook. Returns True if sheet is successfully added.

It's not clear what readOnlyFlag does, even when reading the code. It probably could use a better name and/or a comment. It seems it determines whether the sheet should be replaced if it already exists?

The IsSheet function looks like it should be called IsExistingSheetName.

The instructions separator (colon) after With ThisWorkbook: serves no purpose and should be omitted; the Exit Function is redundant, too, since the code branches out of that block anyway.

There's an execution path where you're verifying whether the sheet name exists twice - I'd factor that check out of the If block.

As for the return value - I'd go even more explicit than that:

Dim result As Boolean

Dim isExisting As Boolean
isExisting = IsExistingSheetName(shtName)

If readOnlyFlag And isExisting Then
    result = False
ElseIf Not readOnlyFlag Then
    If isExisting Then DeleteSheet (shtName)
    With ThisWorkbook
        .Sheets.Add(After:=.Sheets(.Sheets.Count)).Name = shtName
    End With
    result = True
End If

AddSheet = result

Now, how helpful is that? How does the calling code access that new worksheet now? With Application.ActiveSheet? That's a dangerous assumption to make!

How about returning the Worksheet object instead? The function would return Nothing if no worksheet is added, and an object reference when it is.

Dim result As Worksheet

Dim isExisting As Boolean
isExisting = IsExistingSheetName(shtName)

If readOnlyFlag And isExisting Then
    'Set result = Nothing '' todo: remove this useless branch
ElseIf Not readOnlyFlag Then
    If isExisting Then DeleteSheet shtName
    With ThisWorkbook
        Set result = .Sheets.Add(After:=.Sheets(.Sheets.Count))
        If Not result Is Nothing Then result.Name = shtName
    End With
End If

Set AddSheet = result

Notice something? The code will blow up if .Add fails, because the function will return Nothing and then .Name cannot be set: you need to handle this.


Populating a list with user input I am trying to learn me some Haskell because I found it to be an interesting language. Right now, I am trying to implement a function in Haskell to get acquainted with the language. The function just gets user input and adds the input numbers to a list of floats.

get_budget_values _values _index _length = do
    tmp_value <- getLine

    let new_value = read tmp_value :: Float
    let new_values = new_value : _values
    let new_index = _index + 1

    if new_index < _length
        then get_budget_values new_values new_index _length
        else return new_values

So, is this a proper way to implement a function like that in Haskell? What beginner mistakes am I making? Are there any standard ways of doing something like this?

Add type signatures to top-level functions

You should always add a type signature to top-level functions:

get_budget_values :: [Float] -> Int -> Int -> IO [Float]
get_budget_values _values _index _length = do
    ...

Make functions easy to use and hard to misuse

However, this function's type is weird. We want to get _length values, so why do we have to supply them first? That's error prone. Our funtion should have the type Int -> IO [Float] instead:

get_budget_values :: Int -> IO [Float]
get_budget_values n = go [] 0 n
  where
    go values index len = do
      tmp_value <- getLine

      let new_value = read tmp_value
      let new_values = new_value : values
      let new_index = index + 1

      if new_index < len
          then go new_values new_index len
          else return new_values

Note that due to get_budget_values' type signature we don't have to use :: Float at read temp_value anymore.

Follow naming conventions and use existing library functions

Still, our function is sub-par, since we don't follow the common naming convention (camelCase) on the one hand don't  use library functions for our advantage.

So let's say we have doNTimes :: Int -> IO a -> IO [a] so that doNTimes n action repeats the action n times and collects the results in a list. Then we can write

getBudgetValues :: Int -> IO [Float]
getBudgetValues n = doNTimes n getSingleFloat
  where
    getSingleFloat = do
       tmpValue <- getLine
       return (read tmpValue)

The getLine followed by read approach is so common that there is a function for that, namely readLn:

    getSingleFloat = readLn

We end up with

getBudgetValues :: Int -> IO [Float]
getBudgetValues n = doNTimes n readLn

What's missing is doNTimes. Now, we know that there is replicate :: Int -> a -> [a], which is almost correct, but we would end up with [IO Float] instead of IO [Float]. But there's also sequence :: [IO a] -> IO [a].*

Therefore, we can write

doNTimes :: Int -> IO a -> IO [a]
doNTimes n action = sequence (replicate n action)

The final result

This function is also often used and available as replicateM in Control.Monad. We end up with

import Control.Monad (replicateM)

getBudgetValues :: Int -> IO [Float]
getBudgetValues n = replicateM n readLn

Exercises

Write replicate and sequence yourself without looking at their respective source code. replicate :: Int -> a -> [a] should fulfil the following properties for positive n:

let result = replicate n x
in length result == n && all (== x) result

For any non-positive n, the list should be empty.

sequence :: [IO a] -> IO [a] should execute the actions and collect their results, e.g.

sequence [putStrLn "Hello", putStrLn "World"]

should print Hello and World on two lines and return [(),()].

* the actual types are more general


Transform Python math game to OOP Background:  I'm trying to learn basic OOP with Python.  I have this program I wrote to help my son practice math problems.  It does addition, subtraction, multiplication, and division.  He can choose easy, medium, or hard for each operator.

Here's an excerpt of the code.  It's the "easy" addition function. (The "medium" and "hard" functions simply choose bigger random numbers.)

def addition_problems_1():
    global name
    score = 0

    while score < 30:
        a = randint(1,10)
        b = randint(1,10)
        sum = a + b

        answer = int(raw_input("%i + %i = " % (a, b)))

        if answer == sum:
            score = score + 1
            print "Good job. Current score is %d" % score

        elif answer != sum:
            print "Oops, the correct answer is %i. Try another one." % sum

    print "Good job, %s. You passed this course!" % name
    enter_lobby()

And here's the "easy" Subtraction function.  (The multiplication and division functions are very similar.)

def subtraction_problems_1():
    global name
    score = 0

    while score < 30:
        a = randint(1,10)
        b = randint(1,10)

        if a > b:
            sum = a - b
            answer = int(raw_input("%i - %i = " % (a, b)))
        else:
            sum = b - a
            answer = int(raw_input("%i - %i = " % (b, a)))

    if answer == sum:
        score = score + 1
        print "Good job. Current score is %d" % score

    elif answer != sum:
        print "Oops, the correct answer is %i. Try another one." % sum

    print "Good job, %s. You passed this course!" % name
    enter_lobby()

My Question:  Could this script be shortened and/or simplified by re-writing it in OOP style?  If so, can you show me an example of what that might look like?

It is rather difficult to provide a complete review of the code without having access to it as a whole. For instance, the global name line in both functions is rather bad. Firstly because you're not modifying but just accessing name's value so you don't need it. Secondly because using this kind of global variables is bad anyway, and this might have different ways of solving depending on what you're doing with name in other parts of your code (to which OOP might be a way, but not necessarily the way).

Abstract the problem

Do not repeat yourself, it is bad practice and impair maintainability. Instead try to extract a pattern and parametrize it with the different values you need. Not accounting for the if a > b test in the substraction for now, the only thing that differ between the two functions is:

The operation to perform (both taking two arguments);
The representation of this operation.

You can thus define

def problems_1(operation, symbol):
    score = 0

    while score < 30:
        a = randint(1,10)
        b = randint(1,10)
        result = operation(a, b)

        answer = int(raw_input("%i %s %i = " % (a, symbol, b)))

        if answer == result:
            score = score + 1
            print "Good job. Current score is %d" % score

        elif answer != result:
            print "Oops, the correct answer is %i. Try another one." % result

    print "Good job, %s. You passed this course!" % name
    enter_lobby()

And call it using one of three ways:

def add(a,b):
    return a + b
def sub(a,b):
    return a - b
...
problems_1(add, '+')
problems_1(sub, '-')

problems_1(lambda a,b: a+b, '+')
problems_1(lambda a,b: a-b, '-')

import operator

problems_1(operator.add, '+')
problems_2(operator.sub, '-')

Using the operator module is neater since it allows you to reuse existing code.

Account for sorting operands

We left aside the problem of sorting the operands for the substraction problem. This can be incorporated back using a third parameter to tell the function whether or not we should try to sort the operands:

def problems_1(operation, symbol, sort=False):
    score = 0

    while score < 30:
        a = randint(1,10)
        b = randint(1,10)

        if sort and a < b:
           a, b = b, a   # Swap variables using tuple unpacking

        result = operation(a, b)

        answer = int(raw_input("%i %s %i = " % (a, symbol, b)))

        if answer == result:
            score = score + 1
            print "Good job. Current score is %d" % score

        elif answer != result:
            print "Oops, the correct answer is %i. Try another one." % result

    print "Good job, %s. You passed this course!" % name
    enter_lobby()

You can then call your function using

import operator

problems_1(operator.add, '+')
problems_1(operator.sub, '-', True)

You can also choose to sort unconditionally, it won't change the expected output for + or *; and it will force results to be greater than 1 for / (asuming your “divisions” functions ask for integral division). But having this optional parameter can come in handy when dealing with more difficult problems that can potentially benefit from having substraction produce negative results.

Expand on greater difficulty

I guess that the _1 at the end of the function name is for “easy” and that you have _2 and _3 types of functions which all look the same.

Again, don't repeat yourself¹. Parametrize your function instead. If all you need to change is the range of the randint calls, then you should use:

def problems(operation, symbol, min_op, max_op, sort=False):
    score = 0

    while score < 30:
        a = randint(min_op, max_op)
        b = randint(min_op, max_op)

        if sort and a < b:
           a, b = b, a   # Swap variables using tuple unpacking

        result = operation(a, b)

        answer = int(raw_input("%i %s %i = " % (a, symbol, b)))

        if answer == result:
            score = score + 1
            print "Good job. Current score is %d" % score

        elif answer != result:
            print "Oops, the correct answer is %i. Try another one." % result

    print "Good job, %s. You passed this course!" % name
    enter_lobby()

And that's it. This single tiny change allows you to remove at least 8, all look-alike, functions.

The call then becomes:

import operator

problems(operator.sub, '-', 1, 10, True) # easy
problems(operator.sub, '-', 10, 100, True) # medium
problems(operator.sub, '-', 100, 1000) # hard

Coding standards

Now that we removed a whole bunch of your code at once, let's write it properly. For starter, you should have noticed that I changed your sum variable into result. This is both because it better express the intent and because sum is a builtin function in Python that you are shadowing. Try to avoid using builtin functions names for your variables, it makes the code less understandable.

Second, the use of % format specifiers is to be replaced with the format string syntax. It is pretty simple for basic use (you don't even need to handle types yourself) and can be much more expressive it need be. For instance "%i %s %i = " % (a, symbol, b) becomes "{} {} {}".format(a, symbol, b).

Next, answer == sum and answer != sum are already complementary clauses. There is no third (or fourth, or whatever) choice. Thus you can use an else instead of your elif.

Last, your code will fail if the user enters something that fails to be an integer. int(raw_input(...)) will raise ValueError in such cases. You could account for that using a try .. except clause and providing a default value to answer (such as None) if the user inputs something unparsable.

The code thus become:

def problems(operation, symbol, min_op, max_op, sort=False):
    score = 0

    while score < 30:
        a = randint(min_op, max_op)
        b = randint(min_op, max_op)

        if sort and a < b:
           a, b = b, a   # Swap variables using tuple unpacking

        result = operation(a, b)

        try:
            answer = int(raw_input("{} {} {} = ".format(a, symbol, b)))
        except ValueError:
            answer = None

        if answer == result:
            score += 1
            print "Good job. Current score is {}".format(score)
        else:
            print "Oops, the correct answer is {}. Try another one.".format(result)

    print "Good job, {}. You passed this course!".format(name)
    enter_lobby()

Rest of the code

In addition to the speech about using global at the beginning of this post, the call enter_lobby() at the end of the function seems suspicious. What I imagine is that this enter_lobby() function calls one of the various “problems” once, and that's it. Making each problem responsible of returning to it.

This is wrong because it is an implicit recursion. And potentially an infinite one. The lobby calls a problem, which calls a lobby, which calls an other problem, which calls a third lobby… See what I mean? The control flow never return to the first lobby, thus consumming memory to manage the call stack and potentially turning into a RuntimeError: maximum recursion depth exceeded.

This also impairs reusability of your code. Each problem should only be responsible of it's own duty and shouldn't have to know about the rest of the code. If you want to make sure that you stay in the lobby at the end of each problem, you should probably put your enter_lobby code into some kind of infinite loop.

This loop being either within enter_lobby or in the caller body does depend on your intent and the rest of your code, though.

¹ Oh, snap!


Using an Angular factory to run data through logic across controllers My service:

myApp.factory("MediaService", ['$http',
function($http) {

    return {
        all: function() {
            return $http.get('/media');
        },
        save: function( data ) {
            return $http.post('/media', data);
        }
    }
}
]);

My controller:

myApp.controller("MediaController", ['$http', '$scope', '$routeParams', 'Upload', 'MediaService', 'messageService',
function( $http, $scope, $routeParams, Upload, MediaService, messageService) {
    var media = this;

    MediaService.all().success( function (data) {
        media.versioning = []
        media.files = []
        for(i = 0, len = data[0].file.length; i< len; i++)
            if(!media.versioning[data[0].file[i].key[0]] || media.versioning[data[0].file[i].key[0]].version[0] < data[0].file[i].version[0])
                media.versioning[data[0].file[i].key[0]] = data[0].file[i]
        for(var value in media.versioning)
            media.files.push( media.versioning[value] )
    });

I want to move the code within .success() into the MediaService factory so I can re-use it in another controller.

Obviously I do not want to simply copy and paste this into the other controller, although that would work.

I tried to $q.defer/resolve within the service but it did not work (I believe that practice is deprecated).

What is the best way to move this logic into the service? Ideally I would like to be able to use media.files = MediaService.files().

You could define your 'service' on the service provider instead of the factory, or put one in between. Any property exposed on the service can be referenced in the controller, and whatever the service does with that property will be reflected in the controller. That's one of the differences between factory and service.

Take a look at this plunker for an example.


Pipeline operator in scala Please, review my implementation of the F# pipeline operator in Scala

class PipelineContainer[F](value: F) {
  def |>[G] (f: F => G) = f(value)
}

implicit def pipelineEnrichment[T](xs: T) = new PipelineContainer(xs)

  test("Find the last but one element in the list") {
    // example: penultimate(List(1,2,3,3,4,5))
    // result: 4

    def reverse[T](list: List[T]) = list.reverse

    def head[T](list: List[T]) = list.head

    def tail[T](list: List[T]) = list.tail

    def lastButOne[T](list: List[T]) = list |> reverse |> tail |> head
  }

There isn't much that can be reviewed. If you use Scala 2.10 then you should use an implicit class instead of an implicit conversion. Even better: use an implicit value class:

implicit class PipelineContainer[F](val value: F) extends AnyVal {
  def |>[G] (f: F => G) = f(value)
}

Furthermore, in Scala, type parameter are enumerated starting with an A, but this is more a style issue.


Parsing a component of a regex followed by an optional quantifier character I have a piece of parsec code that parses a unit optionally followed by ?, + or *. Depending on which type I build my Regex (this is a part of a regex parser) as follows:

factor = unit >>= (\x -> (try (char '?' >> return (Or Epsilon x))
                         <|> try (char '+' >> return (Then x (Star x)))
                         <|> try (char '*' >> return (Star x))
                         <|> return x))

This is ugly, so is the do version:

factor = do
    x <- unit
    (try (char '?' >> return (Or Epsilon x))    <|>
     try (char '+' >> return (Then x (Star x))) <|>
     try (char '*' >> return (Star x))          <|>
     return x)

Please help me make it simpler.

Whenever I find myself using a lambda, I usually replace it with a local definition:

factor = unit >>= factor'
 where
  factor' x
    =  try (char '?' >> return (Or Epsilon x))
   <|> try (char '+' >> return (Then x (Star x)))
   <|> try (char '*' >> return (Star x))
   <|> return x

Since you only need to look at one character to pick the right branch, you don't need to use try everywhere:

factor = unit >>= factor'
 where
  factor' x
    =  (char '?' >> return (Or Epsilon x))
   <|> (char '+' >> return (Then x (Star x)))
   <|> (char '*' >> return (Star x))
   <|> return x

Finally, we can get rid of the extra parens by using *> instead of >>, and we can replace return with pure:

factor = unit >>= factor'
 where
  factor' x
    =  char '?' *> pure (Or Epsilon x)
   <|> char '+' *> pure (Then x (Star x))
   <|> char '*' *> pure (Star x)
   <|> pure x

This works because *> has higher precedence than <|> which has higher precedence than >>.

I find this easier to read. We've restricted the bulk of our code to Applicative operations, and we only need to use Monadic bind at the beginning where factor' has a direct dependency on unit.

EDIT: We don't need try anymore because char x only consumes a character if the next character in the input stream matches x. Furthermore, each of your branches can either completely match or it can completely fail, and there's no danger of overlap between rules. As an example, here's a simple parser that, like yours, has simple match-or-fail parsers that consume only one character and do not overlap:

data Foo = One | Zero
  deriving (Eq, Show)

foo =  char '1' *> pure One
   <|> char '0' *> pure Zero

If we decide to add a new rule that does overlap, we'll have a problem:

data Foo = One | Zero | Ten
  deriving (Eq, Show)

foo =  char '1' *> pure One
   <|> char '0' *> pure Zero
   <|> char '1' *> char '0' *> pure Ten

Now our first and last rule overlap! We can never successfully parse a Ten:

> parse foo "" "1"
Right One
> parse foo "" "10"
Right One

One's first instinct might be to switch the order of the rules, but this just introduces a new problem:

foo =  char '1' *> char '0' *> pure Ten
   <|> char '0' *> pure Zero
   <|> char '1' *> pure One

> parse foo "" "10"
Right Ten
> parse foo "" "1"
Left (line 1, column 2):
unexpected end of input
expecting "0"

Since the first char '1' has already consumed a character, we've committed to that branch. This is the case where you want to use try; where you might need to backtrack after initially committing to some rule that consumes input:

foo =  try (char '1' *> char '0' *> pure Ten)
   <|> char '0' *> pure Zero
   <|> char '1' *> pure One

> parse foo "" "0"
Right Zero
> parse foo "" "1"
Right One
> parse foo "" "10"
Right Ten

Because many parsers are more complicated than char, it can be easy to just add try without thinking. This can make your parser less efficient because it may have to backtrack many times before it makes progress. It can be worth your time to investigate whether a try is really necessary.


Simple endless obstacle dodging game I've made a quick game in C++ where you race down a vertical track and dodge 'X's or obstacles. The player is represented as a 'v', and the score is counted by how far you manage to get down the track. The obstacles are randomly generated at the bottom of the map and slide up each time the player moves (using the arrow keys).

I have the dodgeGame() function linked from another source, so that's where the program actually starts.

How can I make it better?

#include <conio.h>
#include <cstdio>
#include <cstdlib>
#include <iostream>
#include <time.h>
using namespace std;

void restartMap(char map[9][9]) //Sets all values on map to ' '
{
        for(int i = 0; i < 9; i++)
    {
        for(int g = 0; g < 9; g++)
        {
            map[i][g] = ' ';
        }
    }
}

void drawMap(char map[9][9]) //Displays full array, with lines on each side
{
    for(int i = 0; i < 9; i++)
    {
        cout << '|';
        for(int g = 0; g < 9; g++)
        {
            cout << map[i][g];
        }
        cout << '|' << endl;
    }
}

void slideMap(char map[9][9], int player) //Slides each value in the array up, adjusts bottom row
{
    for(int i = 0; i < 8; i++)
    {
        for(int g = 0; g < 9; g++)
        {
            map[i][g] = map[i+1][g];
        }
    }
    map[0][player] = 'v';
    for(int i = 0; i < 9; i++)
    {
        if(rand() % 6 == 0) //One in six chance to place an obstacle on the bottom row, otherwise blank space
        {
            map[8][i] = 'X';
        }
        else
        {
            map[8][i] = ' ';
        }
    }
}

void mapInfo(int player, int count)
{
    cout << "Position: " << player << endl << "Distance Traveled: " << count;
}

void dodgeGame() //MAIN
{
    srand (time(NULL));
    char map[9][9];
    restartMap(map);
    int player = 4;
    map[0][player] = 'v';
    int c = 0, count = 0;
    drawMap(map); mapInfo(player, count);
    bool crash = false;
    while(true)
    {
        c = getch();
        switch(c)
        {
            case 224:   //Issue with error keys
            continue;

            case 80: //down
            if(map[1][player] == 'X') crash = true;   //These test if there is an obstacle where the player is trying to move to
            break;

            case 75: //left
            if(player == 0) continue;
            if(map[1][player-1] == 'X')
            {
                crash = true;
                break;
            }
            map[0][player] = ' ';
            player--;
            map[0][player] = 'v';
            break;

            case 77: //right
            if(player == 8) continue;
            if(map[1][player+1] == 'X')
            {
                crash = true;
                break;
            }
        map[0][player] = ' ';
        player++;
        map[0][player] = 'v';
        break;

        default: continue;
    }
    count++;
    system("cls");
    slideMap(map, player);
    if(crash == true) map[0][player] = '=';
    drawMap(map); mapInfo(player, count);
    if(crash == true) break;

    }
cout << "\n|<|Crash!|>|\nFinal Score: " << count << ".\n";
}

The first change I'd make would be to get rid of most of the 9s you currently have sprinkled throughout the code.

static const int rows = 9;
static const int cols = 9;

This way if you decide you want a 10x10 grid (or 8x11, etc.) it should be pretty easy to do that.

The second possibility would consider would be a (probably somewhat misplaced) attempt at optimization. In particular, instead of copying the rows up after each turn, I'd keep track of which row is the current starting point at any given time. Each update would just advance that, modify the row that's now the "bottom", and re-draw.

Honestly, as long as you're only dealing with an 9x9 grid, that's probably not going to gain enough to notice. For a game that (for example) was drawing 2D graphics instead, it would probably make a rather more meaningful difference.

I'd probably define names for the 80, 75 and 77 in the main loop as well:

enum {LEFT = 75, RIGHT = 77, DOWN = 80 };

// ...

switch (ch) {
    case LEFT:
        if(player == 0) continue;
        if(map[1][player-1] == 'X')
        {
            crash = true;
            break;
        }
        map[0][player] = ' ';
        player--;
        map[0][player] = 'v';
        break;

The name player should probably also be changed to something like horizontal_position or current_column (at least I think that's what it represents).

You might also consider re-casting the code to be a map class, with reset (sounds more accurate than restart to me), draw, slide, and info as member functions. If you're going to do this, you might also consider getting rid of the reset completely. Instead, do that initialization in the constructor, and when you want to clear the grid, you just create a new map object.

One other minor detail: if(crash == true) should normally be written just as if (crash) instead.


Subset sum whose set contains only positive integers I was trying to write a dynamic programming algorithm using a bottom up approach that solves the subset sum problem's version where the solution can be either an empty set and the initial set can only contain positive integers.

The following is my implementation, but I am not sure it is correct for all cases.

def _get_subset_sum_matrix(subset, s):
    m = [[0 for _ in range(s + 1)] for _ in range(len(subset) + 1)]

    for i in range(1, s + 1):
        m[0][i] = 0

    for j in range(0, len(subset) + 1):
        m[j][0] = 1

    return m

def subset_sum(subset, s):
    m = _get_subset_sum_matrix(subset, s)

    for i in range(1, len(subset) + 1):

        for j in range(1, s + 1):

            if subset[i - 1] == j:
                m[i][j] = 1
            else:
                # We can include the current element,
                # because it is less than the current number j.
                if subset[i - 1] <= j:
                    m[i][j] = max(m[i - 1][j], m[i - 1][j - subset[i - 1]])
                else:
                    m[i][j] = m[i - 1][j]

    return m[-1][-1]

You can imagine the idea of my algorithm as follows.

I have the numbers of the set in the vertical axis on the left, where the first element is actually the empty set. These numbers are not considered as only numbers, but, as I go down from the empty set (the first element), I start considering greater sets, that include all previous elements plus the current one.

Example, suppose I have the set S = {1, 2, 3}. I first consider the empty set, then the union of the empty set and {1}, then the union of {1} and {2}, and finally the union of {1, 2} and {3}.

In the horizontal axis you can imagine I have an increasing sequence of numbers up to the number we want to obtain (by summing the numbers of a certain subset of S). Example, suppose we want to obtain 4, then the increasing sequence would be 0, 1, 2, 3, 4.

So, I first start considering I want to obtain the number 0, and then 1, 2, etc, as it is usually done in a dynamic programming algorithm using a bottom-up approach.

Apart from the setup of the matrix, my algorithm assigns 1 to m[i][j], for some i = 0, 1, ..., N, where N is the size of the set S, and for some j = 0, 1, ... , M, where M is the number we want to obtain, when either the current number in the subset, that is S[i - 1], is equal to the number we want to obtain M_j, or when the previous solution to the subproblem, where the number we want to obtain is M_j - S[i - j], was 1.

That might seem a confusing explanation, and I think the code is self-explanatory.

Is my algorithm correct for all instances of the problem?

Is there a way I can improve it?

First of all I, for one, do not think your code is self-explanatory. I've read it quite a few times, and if I do understand the code you've written, but I still fail to properly understand your logic. In other words, your code would benefit greatly from some test cases, with proper explanations around them. Even a link to which algorithm you are attempting to implement would be better.

So here are some other stylistic and code review items:

Spacing is good, but could have more comments – I like both your vertical and horizontal spacing. This renders the code easy, but I do miss some comments here and there to indicate more of the algorithmic meaning of your code.
Somewhat good naming – You are not very fond of long names, I gather, and some of the like the loop indices, i, j, I don't mind. And I kind of understand the m for matrix, but the s is just a little random. I would consider at least explaining some of the latter ones even if you keep them short for brevity of the code.
Function naming is somewhat unclear – I would rather use build_subset_sum_matrix() instead of get, as you don't simply fetch a value, you do build an entirely new matrix.

The same applies for the subset_sum, it is not quite clear what this function actually does. It does something related to subsets and sum, but what?
Don't understand your range's – Using range(1, s + 1) seems kind of strange, and I think I would have used range(s) instead. Similarily later down you do range(1, len(subset) + 1), when you could have done range(len(subset)). This would of course change that instead of i and i - 1 you do i - 1 and i, or similar.
Why reset 0 to 0? – Within your first function you first set all element to 0, and then you reset  m[0][i] = 0. Why?  (I would also consider switching around i and j to match the other function...)

In fact you simplify the entire function to a list comprehension like the following:

def build_subset_sum_matrix(subset, size):
    return [[0 if i > 0 else 1 for i in range(size + 1)]
            for _ in range(len(subset) + 1)]

Most of these comments are minor issues, and some personal preferences, as the main issue you are trying to solve still alludes me somewhat. In other words, I'm not sure that your code actually does what you want it to do, and that it is the optimal solution.

Add testing

To accommodate this I would strongly suggest to add multiple tests to your code to verify correctness. Try to vary these tests as much as possible to cover most edge cases, and use them to further explain your algorithm.

There are loads of variations, and I'll only suggest two of the more standard approaches. Namely that of the modules unittest and doctest.

The latter one allows stuff like the following variant of the modified build subset sum matrix:

    def build_subset_sum_matrix(subset, size):
        """Initialize a matrix with 1's and 0's.

        >>> build_subset_sum_matrix([1, 4, 10], 4)
        [[1, 0, 0, 0, 0], [1, 0, 0, 0, 0], [1, 0, 0, 0, 0], [1, 0, 0, 0, 0]]
        """
        return [[0 if i > 0 else 1 for i in range(size + 1)] for _ in range(len(subset) + 1)]

This could be run always by doing an import doctest, and then a doctest.testmod() somewhere in your file. If the test runs expectedly, you don't see anything, but if the wrong result is generated you get some error messages. See more information in the links given above.


Python 3 - simple temperature program I have been teaching myself Python (as well as Tkinter) and today one of the sites I was reading, asked us to write a program to covert one value from Fahrenheit to Celsius, once - an easy three line program. As you can read in the comments for the program below, I wanted to expand on this.

This program is one of the most complex and best organized for me thus far. I have feed it through a PEP8 program and it returned no errors, and the program itself runs as I intended and I haven't crashed it when passing errors.

I would be grateful for any comments on the structure of the program, as well as areas I can improve and/or streamline the code - any and all constructive feedback is welcomed!

#!/usr/bin/python
"""
Program: Temperature Coversion (C to F, or F to C)
Date:    02 May 2019
Author:  Jason P. Karle
Remark:  This program was inspired by a Python exercise that
asks you to create a program that will convert one Celsius value to Fahrenheit;
so a program that can be executed with three lines of code.
However, I wanted to make something that would allow the user to
convert to and from either C of F, and do so multiple times, until the user
decides to end the program. This was also an exercise for me to
advance not only my code skills, but how I structure a program.
"""

def quitContinue():
    print("\nDo you want to:\n")
    print("     1. Make another conversion; or")
    print("     2. Exit the program?\n")
    answer = input("Make you selection: ")
    try:
        if answer == "1":
            mainProg()
        else:
            return
    except:
        print("That is not a valid choice.")
        quitContinue()

def CtoF_Calc():
    print("\nThank you, please enter the")
    print("value you want to convert.")
    print("Enter a value between -273.5°C to")
    print("+5.5 dectillion °C")
    value = float(input(": "))
    try:
        if value < -273.5 or value > 5.5**30:
            print("That is not a valid range.")
            celciusCalc()
        else:
            answer = (value*(9/5))+32
            print(f"{value}°C equals: {answer}°F")
            quitContinue()
    except:
        print("Please entet a number!")
        CtoF_Calc()

def FtoC_Calc():
    print("\nThank you, please enter the")
    print("value you want to convert.")
    print("Enter a value between -273.5°C to")
    print("+5.5 dectillion °C")
    value = float(input(": "))
    try:
        if value < -459.5 or value > 42**30:
            print("That is not a valid entry.")
            celciusCalc()
        else:
            answer = (5/9)*(value-32)
            print(f"{value}°F equals: {answer}°C")
            quitContinue()
    except:
        print("That is not a number!\n")
        FtoC_Calc

def makeSelection(selection):
    try:
        if selection == "1":
            CtoF_Calc()
        elif selection == "2":
            FtoC_Calc()
        else:
            return
    except:
        print("That is not a valid selection")
        makeSelection(selection)

def mainProg():
    print("Please enter the number")
    print("corresponding to what you")
    print("want to convert:")
    print("     1. Celcius to Farenheit")
    print("     2. Farenheit to Celcius")
    print("     3. Exit\n")
    selection = input("Enter 1, 2 or 3: ")
    makeSelection(selection)

if __name__ == "__main__":
    print("Welcome to the temperature")
    print("conversion program!\n")
    mainProg()

At first glance your code looks nice and clean. And even though the naming goes against PEP8 would be near perfect style.

I then entered your code into PyCharm, and then and there I knew this was unfortunately a facade.

Firstly I don't think you've set your linter up correctly. As within seconds I knew your code wasn't PEP8 compliant due to using camel case function names.

  I have feed it through a PEP8 program and it returned no errors

PEP 8 convention is to use snake_case rather than quitContinue or CtoF_Calc.
You have a couple of miss-spelt words, Prog and "entet".
It's advised against having bare except statements as they catch too many errors and lead to hard to debug errors. This can actually be seen in your program, it's hidden a bug that you probably don't know about.
celciusCalc is undefined, and so is a bug in your program.
PEP8 and linters are quite finicky when it comes to whitespace. value*(9/5) should have spaces either side of the operators.
You have a 'pointless statement', FtoC_Calc. This is as you forgot to call the function. And so causes a bug in your program.

And so I recommend you look into configuring your linter to get the maximum amount of warnings possible. I personally use Prospector and Flake 8 with a butt tone of plugins.

In FtoC_Calc you state the range in celsius, which is confusing. If I'm using your program I likely won't know what the equivalent in Fahrenheit is.
Keep the code in the try statement to be as small as possible. Looking at the except it looks like the purpose is to handle when you don't enter floating point integer values.

You haven't put the call to float in the try and so you have another bug.
You should use except ValueError.
You can put all the code that is currently in the try in an else statement.
You should replace celciusCalc() with FtoC_Calc and CtoF_Calc.

Currently your design is sub-optimal, FtoC_Calc interacts with the user and performs the mathematical calculations.

It also uses recursion rather than loops to cycle through the function calls, leading not only to spaghetti code, but toward getting a RuntimeError where you exceed the recursion depth.

This is all rather easy to deal with. Split the function into three distinct functions:

One that gets a floating point integer from the user. And handles interactions with the user if they enter an incorrect value.

If a user enters an incorrect value you'll want to have your try statement in a while loop to continuously ask for input.

By giving a range or an 'is_invalid' function to this function you can reduce the amount of duplicated code.
The function to convert C to F.
The function that calls both of these functions.

The majority of your code in make_selection is not needed. No exceptions should be raised from these functions, and it isn't the correct place to handle them.
quit_continue should be changed to a function that returns a boolean. This can be used in mainProg to determine if the user will continue using the program or exit.

This means mainProg should contain a while loop to continuously allow the user to enter values they want to convert.

It should be noted that quit_continue shouldn't need the try and should never reach the except. However adding more code the way you did would make this assumption to be less safe as the program becomes more and more problematic.
I changed your string delimiters as one of my tools errors on ", as I commonly use '. Using " is perfectly acceptable.

#!/usr/bin/python
"""
Program: Temperature Coversion (C to F, or F to C)
Date:    02 May 2019
Author:  Jason P. Karle
Remark:  This program was inspired by a Python exercise that
asks you to create a program that will convert one Celsius value to Fahrenheit;
so a program that can be executed with three lines of code.
However, I wanted to make something that would allow the user to
convert to and from either C of F, and do so multiple times, until the user
decides to end the program. This was also an exercise for me to
advance not only my code skills, but how I structure a program.
"""

def input_float(prompt, is_invalid):
    while True:
        try:
            value = float(input(prompt))
        except ValueError:
            print('That is not a number!')
        else:
            if is_invalid(value):
                print('That is not a valid number.')
                continue
            return value

def c_to_f(value):
    return (value * (9 / 5)) + 32

def f_to_c(value):
    return (5 / 9) * (value - 32)

def convert_c_to_f():
    print('\nThank you, please enter the')
    print('value you want to convert.')
    print('Enter a value between -273.5°C to')
    print('+5.5 dectillion °C')
    celsius = input_float(': ', lambda v: v < -273.5 or 5.5**30 < v)
    fahrenheit = c_to_f(celsius)
    print(f'{celsius}°C equals: {fahrenheit}°F')

def convert_f_to_c():
    print('\nThank you, please enter the')
    print('value you want to convert.')
    print('Enter a value between -459.5°F to')
    print('+42 dectillion °F')
    celsius = input_float(': ', lambda v: v < -459.5 or 42**30 < v)
    celsius = f_to_c(fahrenheit)
    print(f'{fahrenheit}°F equals: {celsius}°C')

def quit_continue():
    print('\nDo you want to:\n')
    print('     1. Make another conversion; or')
    print('     2. Exit the program?\n')
    answer = input('Make you selection: ')
    return answer == '1'

def main():
    while True:
        print('Please enter the number')
        print('corresponding to what you')
        print('want to convert:')
        print('     1. Celsius to Fahrenheit')
        print('     2. Fahrenheit to Celsius')
        print('     3. Exit\n')
        selection = input('Enter 1, 2 or 3: ')
        if selection == '1':
            convert_c_to_f()
        elif selection == '2':
            convert_f_to_c()
        else:
            return
        if not quit_continue():
            return

if __name__ == '__main__':
    print('Welcome to the temperature')
    print('conversion program!\n')
    main()


Grouping reviews by UserId The class I'm using is called Review and looks like this:

public class Review
{
    public virtual Guid Id { get; set; }

    public virtual int Rating { get; set; }

    public virtual Guid RestaurantId { get; set; }

    public virtual Guid UserId { get; set; }
}

I'm using a LINQ statement to first check a few things and afterwards group by a user's reviews. I'll explain what I mean with this:

var reviewsByUsers =
    reviews
    .GroupBy(x => x.RestaurantId)
    .Where(x => x.Count() >= 3)
    .SelectMany(x => x.ToList())
    .Distinct()
    .GroupBy(x => x.UserId)
    .Where(x => x.Count() >= 2)
    .Select(x => x.ToList())
    .ToList();

And now step by step:

1. reviews is a list of all reviews given on a bunch of restaurants. This is not the database, it's a list I receive as a parameter of the method.

2. .GroupBy(x => x.RestaurantId) Group these reviews per each restaurant. This is for counting them later.

3. .Where(x => x.Count() >= 3) Exclude any restaurants which don't have 3 or more reviews on them.

4. .SelectMany(x => x.ToList()) Flatten the list so we can group again.

5. .Distinct() To be sure that I'm not getting doubles.

6. .GroupBy(x => x.UserId) Group the reviews per user. This is for counting them later.

7. .Where(x => x.Count() >= 2) Exclude any user which has less than 2 reviews.

8. .Select(x => x.ToList()) I don't need anything else per user, a list of reviews per user is enough.

All of this will result in a List<List<Review>> which is a list that contains a list of all reviews grouped by the UserId.

I'm certain there has to be a better way to perform these checks and create a list of lists grouped by user id. I'm just very new to C#, so I don't know how to increase this LINQ statement.

There is actaully another solution that utilizes the Except extension. This means you first select all reviews with at least two users but remove those where the restaurant does not have at least three reviews.

This requires to write a new IEqualityComparer that I find is an overkill for this but on the other hand the Except clearly tells what kind of results we don't want to have. Anyway, the alternative solution:

reviews
    .GroupBy(r => r.UserId)
    .Where(g => g.Count() > 1)
    .Except(
        reviews
            .GroupBy(r => r.RestaurantId)
            .Where(g => g.Count() < 3),
        new ReviewComparer()
    );

where

class ReviewComparer : IEqualityComparer<IGrouping<Guid, Review>>
{
    public bool Equals(IGrouping<Guid, Review> left, IGrouping<Guid, Review> right)
    {
        // left - user-group
        // right - restaurant-group
        // exclude the user group
        // if it contains any restaurant-id of the excluded restaurant-ids
        return left.Any(x => right.Any(y => y.Id == x.Id));
    }

    public int GetHashCode(IGrouping<Guid, Review> reviews)
    {
        return 0; // treat all groups equally
    }
}


Subclass Tkinter Widgets to save typing I am doing a small GUI program for a practice exercise and in order to save myself some typing I took it upon myself to subclass some Tkinter widgets so I can pass the grid placement in the instantiation.  One of my subclasses looks like this:

import tkinter as tk
class EntryBox(tk.Entry):
    def __init__(self, master=None, width=1, relief=None, row=None, column=None, sticky=None, padx=None, pady=None, **kwargs):
        super().__init__(master=master, width=width, relief=relief, **kwargs)
        self.grid(row=row, column=column, sticky=sticky, padx=padx, pady=pady)

And one of my instances of this widget looks like this:

window = tk.Tk()

title_entry = EntryBox(window, 10, 'sunken', 0, 1)

window.mainloop()

My initial thinking for the subclass is that if I am creating these entry widgets, they will clearly be visible on the screen (similar for some Labels, text areas, buttons, etc...).  So this would save many lines of code and typing.

Is it appropriate to subclass these widgets to save typing, or does this typically cause more confusion when another programmer reads it?  Also, will this be considered more error-prone? (Perhaps it would still be best to pass named parameter in my instantiation?)

I'd like to get some comments on when it's good practice / appropriate to subclass and when it causes more confusion.

I wanted to add that I decided to pass default values in the init subclass method so that if someone wanted to call the grid methods in the traditional way they still could in the code.

It is perfectly normal to attempt to subclass a Tkinter widget to create a custom one. Your choice is valid and justified.

However, as Tkinter does not have a EntryBox() class, it would be better to rename this, in order not to confuse the reader of your code, to rename it to something like: CustomEntry() or, why not, CustomEntryWidget().


Distribution problem optimization The situation is this: I have a bunch of servers which are synced so that they all have the same data. Unfortunately, a disaster happened and the servers are all out of sync. My task is to re-sync all of the servers so that they all have the same sets of data. That is, I need to ensure that each server has a copy of every data set.

The first line of input denotes the number of servers. Following that will be one line of input for each server with a space-separated list of data set ID's. i.e, x y z. Data set id's are positive integers.

I must output a list of instructions to most optimally re-sync the servers in the format of: < data set ID >< FROM >< TO >

#input = {
#1:[1, 3, 4],
#2:[1, 2, 3],
#3:[1, 3],
#4:[1, 4, 2]}

numCenters = int(raw_input('Number of data centers: '))

input = {}
print("Input data set information as: x y z")

# Grab dataset ID information from stdin
for x in range(1, numCenters+1):
        dataSet = raw_input("Data set %s: " % (x))
        input[x] = sorted(map(int, dataSet.split()))

#Map datasets (the numbers / dataset ID) to data centers (the individual lists) that they belong to

#New dictionary for the map
alldatasets = {}

for k,v in input.iteritems():
        for dataset in v: #
                if dataset not in alldatasets:
                        alldatasets[dataset] = [] # Make a dictionary with the key as the dataset ID,
                alldatasets[dataset].append(k) # and the value as a list of which datacenters have that value.

allsets = list(alldatasets.keys())

print("One Possible Correct Output:\n")
for Id,datacenter in input.iteritems():
        for sets in allsets: #go through every datacenter, and compare the datasets it has to a list of all datasets.
                if sets not in datacenter:
                        print("%s %s %s" % (sets, alldatasets[sets][0], Id))
print("done")

Input:

4
1 3 4
1 2 3
1 3
1 4 2

Output:

One possible correct output:

2 2 1
4 1 2
2 2 3
4 1 3
3 1 4
done

I am looking to improve the code that I have written. Either optimizing its run time, making it look better, or any other comments are welcome. I am moreso looking for optimization feedback, though.

Your code is full of variables named somethingSet — but they aren't actually sets! Why not?

This solution, which takes advantage of Python's built-in set operations, is shorter.  Just being able to write have_nots = all_centers - haves is worth it.

from collections import defaultdict

num_centers = int(raw_input('Number of data centers: '))
print("Input data set information as: x y z")

all_centers = set(xrange(1, num_centers + 1))
centers_with_data = defaultdict(set)

# Grab dataset ID information from stdin
for center in range(1, num_centers + 1):
    for data_set in map(int, raw_input("Data set %s: " % (center)).split()):
        centers_with_data[data_set].add(center)

print "One possible solution:\n"

for data_set, haves in centers_with_data.iteritems():
    have_nots = all_centers - haves
    donor = next(iter(haves))   # Pick an arbitrary source
    for acceptor in have_nots:
        print "%d %d %d" % (data_set, donor, acceptor)