Python/Django Class based saving I'm creating multiple Django apps with vote possibilities, so I made an app vote to handle all this votes. In my templates I'm including an ajax-function named vote. To know on which model I'm liking I add app_name and model_name to the vote function (I made some templatetags to get these values). In my views.py I use model = apps.get_model(app_name, model_name) to get the model class. But now I'm worried a hacker could do something with the app_name and model_name values.

vote/ajax.html (only function):

function vote(bool){
    $.ajax({
        type: "post",
        timeout: 8000,
        url: '{% url 'ajax:vote' %}',
        dataType: 'json',
        data: {
            'csrfmiddlewaretoken': getCookie('csrftoken'),
            'model_name': "{{ model|get_model_name }}",
            'app_name': "{{ model|get_app_name }}",
            'voted': bool,
            'id': "{{ model.id }}",
        },
        success: function(data) {
            if (!data.error){
                if (bool){
                    $(".half .fa-thumbs-up").removeClass("far").addClass("fas");
                    $(".half #count").text(parseInt($(".half #count").text()) + 1);
                } else {
                    $(".half .fa-thumbs-down").removeClass("far").addClass("fas");
                    $(".half #count").text(parseInt($(".half #count").text()) - 1);
                }
            }
        }
    });
}

ajax/views.py:

def vote(request):
    try:
        app_name = request.POST.get("app_name")
        model_name = request.POST.get("model_name")
        id = request.POST.get("id")
        votedFor = True if request.POST.get("voted") == "true" else False
    except ValueError:
        return JsonResponse({"error": True})

    model = apps.get_model(app_name, model_name)

    if model is None or id is None:
        return JsonResponse({"error": True})

    try:
        usable_model = model.objects.get(id=id)
    except model.DoesNotExist:
        return JsonResponse({"error": True})

    try:
        usable_model.vote._meta.get_field("votes")
    except FieldDoesNotExist:
        return JsonResponse({"error": True})

    usable_model.vote.vote(request, votedFor)
    return JsonResponse({"error": False})

vote/functions:

def vote(self, request, votedFor):
    if request.user.is_authenticated:
        if not UserVoted.objects.filter(User=request.user, Vote=self).exists():
            UserVoted.objects.create(User=request.user, Vote=self, votedFor=votedFor)
            self._like_or_dislike(votedFor)
            return True
        return False
    ip = get_client_ip(request)
    if ip:
        if not UserVoted.objects.filter(ip=ip, Vote=self).exists():
            UserVoted.objects.create(ip=ip, Vote=self, votedFor=votedFor)
            self._like_or_dislike(votedFor)
            return True
        return False
    return False

def _like_or_dislike(self, votedFor):
    if votedFor is not None:
        Vote.objects.filter(id=self.id).update(votes=F('votes') + 1) if votedFor else Vote.objects.filter(id=self.id).update(votes=F('votes') - 1)
        return True
    return False

I already manipulated app_name and model_name and the server didn't crash but I don't know what a hacker can do. Can he crash my server when he manipulate these values? (maybe "ajax-injection" or something like this?)

Question

  But now I'm worried a hacker could do something with the app_name and model_name values.

  Can he crash my server when he manipulate these values? (maybe "ajax-injection" or something like this?)

I'm not sure what the best approach to this would be. Perhaps it would be wise to define a list of the values that are acceptable for a user to pass there, though maybe that wouldn't be sufficient.

Other review points

function vote(bool){

The name bool is not very descriptive. Given that it is used as the value for the voted parameter a name like voted would be more appropriate. And if your code complies with ecmascript-6 standards you could simply write
    voted

in the list of parameters as a shorthand for voted: voted

The success handler looks like this:

success: function(data) {
    if (!data.error){
        if (bool){
            $(".half .fa-thumbs-up").removeClass("far").addClass("fas");
            $(".half #count").text(parseInt($(".half #count").text()) + 1);
        } else {
            $(".half .fa-thumbs-down").removeClass("far").addClass("fas");
            $(".half #count").text(parseInt($(".half #count").text()) - 1);
        }

There is quite a bit of redundancy in both cases. Also parseInt() calls should pass a radix1.

if (!data.error){
    const thumbClass = '.fa-thumbs-' + bool ? 'up' : 'down';
    $(".half " + thumbClass).removeClass("far").addClass("fas");
    const toAdd = bool ? 1 : -1;
    $(".half #count").text(parseInt($(".half #count").text(), 10) + toAdd);

In the vote function in ajax/views.py there is this line:

votedFor = True if request.GET.get("voted") == "true" else False

This could be simplified to just:

votedFor = request.GET.get("voted") == "true"

1https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt#Octal_interpretations_with_no_radix


A simple multithreaded FileLogger in C++ In order to learn about multithreading programming in C++, I am implementing a basic multithreaded logger.

I use a std::deque to store messages inside a FileLogger class. Each time a thread logs a message, that message is pushed to the back of the deque.

In a separate thread the FileLogger checks if there are any messages in the deque and if so writes them to file.

Access to the deque is guarded by a mutex.

In order to make it easy to log from anywhere, the logger is implemented as a singleton.

Is my code correct? How can it be improved?

// FileLogger.h:
class FileLogger
{
public:
    static void initialize(const char* filePath) { // called by main thread before any threads are spawned
        instance_ = new FileLogger(filePath);
    }
    static FileLogger* instance() { // called from many threads simultaneously
        return instance_;
    }
    void log(const std::string &msg);
private:
    FileLogger(const char* filePath);
    void writeToFile();
    static FileLogger*     instance_;
    std::deque<std::string> messages;
    std::mutex         messagesMutex; // lock/unlock this each time messages is pushed or popped
    std::ofstream               fout;
    std::thread         writerThread;
};
// FileLogger.cpp:
FileLogger* FileLogger::instance_ = nullptr;

void FileLogger::writeToFile() {
    for (;;) {
        std::string message;
        while (messages.empty()) {
            std::this_thread::sleep_for(std::chrono::nanoseconds(10));
        }
        messagesMutex.lock();
        message = messages.front();
        messages.pop_front();
        messagesMutex.unlock();
        fout << message << std::endl << std::flush;
    }
}

FileLogger::FileLogger(const char* filePath) {
    fout.open(filePath);
    std::thread t(&FileLogger::writeToFile, this);
    writerThread = std::move(t);
}

void FileLogger::log(const std::string &msg) {
    std::lock_guard<std::mutex> lg(messagesMutex);
    messages.push_back(msg);
}

Just up front, your code doesn't have any blatant bugs which would prevent its use. There are a few things that you could do to improve it though:

One thing that I'd try to avoid is singletons. These are effectively globals and bring with them almost all their problems. In particular, it prevents isolating code for unit tests. The alternative is that you pass a logger to the constructor of every object that needs it. This is called dependency injection, as a keyword for future research.
Instead of fout.open(path), you could pass the filename to the constructor of fout using the initializer list.
Your logger isn't copyable or assignable due to its members. I'd make that explicit though.
When writing, std::endl already flushes the stream, so the explicit std::flush is redundant.
The biggest issue is the way your writing thread operates. In effect, it wakes up every 10ns (that's 100.000.000 times per second) and looks for work (a.k.a. polling). Firstly, I can't imagine that this fast response time is actually relevant to a logger. If you have 0.1s delay between writing to the logger and seeing the data in the file, that should be enough by far. However, that's not a solution. What you actually want there is a condition variable, which allows you to only wake up the writer thread when actual works needs to be done.
There is a comment lock/unlock this each time messages is pushed or popped, which is already in the right direction. What has helped me in the past is to document which mutex protects which data. It may even be a good idea to put just those parts into a separate struct in some cases, just to make it clearer. Also, for all other members, document who is allowed to access them. For example, fout doesn't require synchronization, because it is exclusively accessed by the writer thread.
A minor fault, but still a fault is also that you access the queue without locking to check if it is non-empty. Since this typically only involves a simple pointer comparison (though that's implementation-defined), it will probably not cause issue. Using a condition variable as sketched below fixes this part as well.
You could implement this using lock-free algorithms. This is a complete rewrite though and not "smoothing out the rough edges" of your current approach. Just keep that in mind as future research project perhaps.
A minor flaw is that there is no way to shut down the logger. This is probably harmless, but it will probably trigger memory debuggers (leak detectors). If you decide not to implement shutdown, you don't need writerThread though, just call t.detach() after starting.
If your target is C++ 17 or later, you could also use the filesystem library which has a dedicated path type. I'd generally prefer that over raw char* string. Even without C++ 17, I'd use std::string, unless I have good reasons not to.
Concerning pushing on the queue, consider using emplace_back() instead, to avoid one more copying operation.
Concerning removal from the queue, there is a trick: Create a second, local queue (initially empty) and swap that with the member (holding a lock for the time of that operation, of course). Then you have all the content in a local object that you can process at leasure, without having to worry about locking. This becomes important when retrieving multiple messages at once, so that you don't repeatedly lock/unlock without need. This can lead to "thread convoys": Imagine one thread perpetually writing messages using lock/push/unlock triples, so it becomes a stream of lock/push/unlock/lock/push/unlock... Now, if this thread ever gets scheduled out while it holds the lock and the writer thread is scheduled in, it will then itself run a lock/pop/unlock/lock/pop/unlock.. stream. On the first lock, it will block until the logging thread continues. This thread will be able to write one message, before it is blocked by the writer thread. In other words, ownership of the mutex will pingpong between the two threads, wrecking performance. Also, this is hard to catch, because it both comes and goes spontaneously.

For reference, here is a sketch of the push and pop operations using a CV:

mutex mtx;
condition_variable cv;
deque<string> queue;

void push(string s) {
    unique_lock<mutex> lck(mtx);
    queue.push_back(s);
    lck.unlock();
    cv.notify_one();
}

string pop() {
     unique_lock<mutex> lck(mtx);
     cv.wait(lck, []{ return !queue.empty();})
     string res = queue.front();
     queue.pop_front();
     return res;
}

This is not much more than the examples given by cppreference.


Iteratively generating permutations with repetitions I tried to solve the simple problem of generating all permutations of length n with m distinct elements where repetition is allowed. Doing this recursively took me like 10 minutes:

void genAllPermsRec(vector<int>& perm, vector<vector<int>>& perms, int n, int m) {
    if (perm.size() == n) {
        perms.push_back(perm);
        return;
    }
    for (int i = 0; i < m + 1; ++i) {
        perm.push_back(i);
        genAllPermsRec(perm, perms, n, m);
        perm.pop_back();
    }
}

I tried to do it iteratively then and it took me a lot longer. I ended up looking exactly at what happens in the recursive version and came up with the following solution:

vector<vector<int>> genAllPerms(int n, int m) {
    vector<vector<int>> perms;
    vector<int> perm(1, 0);
    int i = 0;
    while (!perm.empty()) {
        if (perm.size() == n) {
            perms.push_back(perm);
            if (perm.back() < m) {
                perm.pop_back();
                ++i;
                continue; // That is why Python has the while: .. else: construct.
            }
            while (perm.back() == m) { perm.pop_back(); }
            if (!perm.empty()) {
                perm.back() += 1;
                i = 0;
            }
        } else { perm.push_back(i); }
    }
    return perms;
}

It seems to work but I am not satisfied with the result. I suspect that there is a more intuitive way to approach the problem that would lead to shorter code. Please enlighten me :)

I think a first problem is that m should be changed to m-1. Or maybe better you should change it so that the lines "i=0;" are changed to "i=1;" instead, and also initializing the vector with 1 instead of 0.

By what I understand a permutation does not necessarily contain each number at least once. So we just need to generate all possible vectors of length \$n\$ with elements in the set \$ \{1,2,\dots, m\}\$.

Your method is the same thing I would do, except I would not use pop_back and instead I would just modify the elements in place (although this is really a small change):

vector <vector <int> > genperms(int n,int m) {
    vector <vector<int>> perms;
    vector <int> curr(n,1);
    perms.push_back(curr);
    while(true){
        int change = 0;
        for(int i=n-1;i>=0;i--){
            if(curr[i] < m){
                curr[i]++;
                for(int j=i+1;j<n;j++){
                    curr[j] = 1;
                }
                change = 1;
                perms.push_back(curr);
                break; //try to change again
            }
        }
        if(change == 0) break; //if we couldnt make a change we are done
    }
    return perms;
}

This modification seems to give like an 8% boost in speed in my computer (using the n=8,m=8 case) . Although probably one can get better improvements if a vector is changed for something else.

If you want to speed this up you can also reserve the outer "perms" vector to a large enough size.


Get result from UI thread to non-UI thread when using await Task.Run I have an application where I use Task to run an operation. During the operation, there can be a problem which will need some user interaction (continue or not). I need to bubble up a messagebox and get the user result back to the operation thread.

This is how I got it working:

public interface IPlugin
{
    void ConnectAndProduce(Func<string, bool> retryRequest);
}

public partial class MainWindow : MetroWindow
{
    [ImportMany(typeof(IPlugin))]
    private IEnumerable<IPlugin> Plugins;

    public MainWindow()
    {
        InitializeComponent();
    }

    private async void Button_Click(object sender, RoutedEventArgs e)
    {
        Func<string, bool> func = message =>
        {
            var result = Application.Current.Dispatcher.Invoke(new Func<Task<MessageDialogResult>>(async () =>
                {
                    var mySettings = new MetroDialogSettings()
                    {
                        AffirmativeButtonText = "YES",
                        NegativeButtonText = "NO",
                        ColorScheme = MetroDialogColorScheme.Accented
                    };

                    return await this.ShowMessageAsync("Hello!", message, MessageDialogStyle.AffirmativeAndNegative, mySettings);
                }));

            if ((result as Task<MessageDialogResult>).Result != MessageDialogResult.Negative)
                return true;

            return false;
        };

        ImportPlugins();
        var plugin = Plugins.FirstOrDefault();

        await Task.Run(() =>
        {
            plugin.ConnectAndProduce(func);
        });
    }

    public void ImportPlugins()
    {
        var catalog = new AggregateCatalog();
        catalog.Catalogs.Add(new DirectoryCatalog(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location)));
        CompositionContainer container = new CompositionContainer(catalog);
        container.ComposeParts(this);
    }
}

And here is the Plugin what is going to do all the work:

[Export(typeof(IPlugin))]
public class Machine : IPlugin
{
    public void ConnectAndProduce(Func<string, bool> retryRequest)
    {
        var connection = MachineConnection();

        var retry = true;
        var retryCount = 0;
        while (retry)
        {
            if (connection != true)
                retry = retryRequest("Do you want to retry?");

            if (retryCount < 3)
                retryCount++;
            else
                retry = false;
        }

        connection.Produce();
    }
}

I use MEF to load the plugin and do all the work in non-UI thread by calling the plugin method in a new Task. Once the plugin has called MachineConnect(), the connection fails and I can't just stop the plugins from working and call it again. I need to continue, and that's why the plugin has to ask if I want to retry.

Is there any better way doing the retry request from plugin to UI thread? This looks very 'hacky' to me.

You're scheduling a new non-UI operation that immediately goes back and schedules an operation to run in the UI thread, which then does some work.

Just avoid all of that, and do the work that you want to do right from the start, since you're in the UI thread:

private async void Button_Click(object sender, RoutedEventArgs e)
{
    var mySettings = new MetroDialogSettings()
    {
        AffirmativeButtonText = "YES",
        NegativeButtonText = "NO",
        ColorScheme = MetroDialogColorScheme.Accented
    };

    var response = await this.ShowMessageAsync("Hello!", "message", MessageDialogStyle.AffirmativeAndNegative, mySettings);

    if(response == MessageDialogResult.Negative)
        return;
}


C++ SFML fonts loader I am writing a game via SFML. In difference places I want to use loaded fonts. So I decided to write the fonts loader, so that load fonts only one time and use them in the future from class. My code:

enum class FontsID
{
    PIXEBOY,
    DIGITAL7
};

class FontObject
{
public:
    FontObject::FontObject(const std::string &texturePath)
    {
        font->loadFromFile(texturePath);
    }
    std::shared_ptr<sf::Font> font = std::make_shared<sf::Font>();
};

class FontsLoader
{
public:
    static FontsLoader &getInstance()
    {
        static FontsLoader instance;
        return instance;
    }
    const std::shared_ptr<sf::Font> getFont(FontsID fontID) const
    {
        return allFonts[static_cast<int>(fontID)].font;
    }

private:
    FontsLoader()
    {
        allFonts.push_back(FontObject("data/fonts/pixeboy.ttf"));
        allFonts.push_back(FontObject("data/fonts/digital-7.ttf"));
    }
    std::vector<FontObject> allFonts;
};
static FontsLoader &fontsLoader = FontsLoader::getInstance();

I don't think using an enum is good solution, for example, enum count may be more or less than allFonts vector items count, adding in the constuctor in proper sequence also not good , but fontsObject.getFont(FontsID::PIXEBOY); is convenient for me, I will never confuse fonts name. Is there a better way?

FontObject

Since all your data members and methods are public, consider making it a struct instead of a class. Makes no difference to a compiler, but makes your intention clearer.
Consider using a unique_ptr instead of shared_ptr, since only one instance of GameObject owns the object.

FontsLoader

I'm sure they are many people who will explain why you shouldn't use the Singleton pattern. Ultimately, it comes down to whether you (and people you're coding with) are okay with. I've seen various large projects (~100K-1M lines) use the Singleton pattern, so I'm not going to tell you whether you should or shouldn't use it.
Use something like an std::unordered_map<FontsID, FontsObject>. That way, you won't have to cast it into an integer and access a vector.
On a side note, for vector access, use size_t.
Consider passing the sf::Font object as a raw pointer or a const reference. Remember, non-owning raw pointers is FINE, even recommended. Whatever calls the getFont method is not concerned with ownership of the object, just the object itself. Don't pass smart pointers around unless you want the calling code to have ownership of the object.


Double checked locking 101 I've found a peace of code I've wrote not long ago. It is used to fetch some dictinary from DB table only once and then return it to requestors. Seems like I tryed to implement double-checked locking here. Is it correct? If not, what are the mistakes?

package a.b.c.service.orders;

import java.util.List;
import java.util.Map;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;
import org.springframework.transaction.annotation.Transactional;

import a.b.c.dao.OrderStatusesDao;
import a.b.c.model.OrderStatuses;

@Component
public class OrderStatusesService {
  @Autowired
  private OrderStatusesDao orderStatusesDao;

  private volatile Map<String, String> cached;

  @Transactional(rollbackFor = Exception.class, readOnly = true)
  public Map<String, String> getAllOrderStatuses() {
    if (null == cached) {
      synchronized (OrderStatusesService) {
        if (null == cached) {
          final List<OrderStatuses> orderStatuses = orderStatusesDao.getAll();

          cached = new HashMap<String, String>();
          for (OrderStatuses orderStatus : orderStatuses) {
            cached.put(orderStatus.getCode(), orderStatus.getName());
          }
        }
      }
    }

    return cached;
  }
}

There are a few reasons why double-checked locking does not always work in Java. There are a number of blogs with more detail, but I quite like this one and this one.

Now, your code contains a volatile declaration for cached which, in certain conditions can mitigate the problem, but it is not a complete solution. You can still have cases where the cached variable is set in one thread, but another thread reads it before it is completely initialized. Since the values in the cached are not volatile, you will have problems still. Also, they depend on orderStatusesDao which is not volatile either...

With Java5's changes to the memory model, you can probably use a 'helper' variable to initialize the data... something like:

      Map<String,String> helper = new HashMap<String, String>();
      for (OrderStatuses orderStatus : orderStatuses) {
        helper.put(orderStatus.getCode(), orderStatus.getName());
      }
      cached = helper;

Using the helper ensures that the cached instance is fully assigned before it gets set.

This all seems very complicated.... and all to save a micro-synchronization.

With the advent of Java6 and enums, there are a few really good ways to create thread-safe lazy-initialized values, but your case is somewhat more complicated by the orderStatusesDao.getAll(); required for initialization.

As an 'aside' comment, I am concerned by this line:

synchronized (OrderStatusesService) { ...

The line suggests you have a variable called OrderStatusesService which is the exact same name as the class OrderStatusesService.... or is the synchronization supposed to be on OrderStatusesService.class?

In your case above, I would probably be satisfied with a simple/standard synchronization block and no volatile declaration. The lock-wait time will be very minimal.

Alternatively, I would consider an optimistic system using AtomicReferences, for example:

Map<String, String> cached = atomicref.get();
if (cached != null) {
    return cached;
}
cached = new HashMap....;
// populate cached.....
if (atomicref.compareAndSet(null, cached)) {
    // we were the first thread to populate, great:
    return cached;
}
// return the value that the winning thread created.
return atomicref.get();

in the above system, the first thread to 'set' the cached value will have it's instance used by every other thread. It is possible that multiple threads may be initializing their values at the same time, but they will all defer back to the winning-thread instance if they don't win. Once the instance is set, all threads will use that instance and there will be only fast atomic locking.


A from_string() function (inverse of std::to_string) I've been thinking of using this:

template <typename T>
inline T from_string(const std::string& s){
   std::istringstream iss;
   iss.str(s);
   T result;
   iss >> result;
   return result;
}

in my code. Then I thought "I shouldn't construct istringstreams all the time", and made it into this:

inline std::istringstream& get_istringstream(){
    static thread_local std::istringstream stream;
    stream.str("");
    return stream;
}

template <typename T>
inline T from_string(const std::string& s){
   auto& iss(get_istringstream());
   iss.str(s);
   T result;
   iss >> result;
   return result;
}

... and this builds and works (although I haven't tested it very extensively, nor ran performance tests). Would you say that "good enough" for general-purpose utility code, that is not intended to run in some tight loop? Are there other considerations I've overlooked, performance-wise (*) or usability-wise?

Perhaps I should mention my motivation here is partly how I found it strange that there's no std::from_string().

Edit: If you're concerned about the dependence on a default constructor, we can also throw in this:

template< typename T >
struct istream_traits {
    inline static T read(std::istream& is)
    {
        T x;
        is >> x;
        return x;
    }
};

template<> struct istream_traits<bool> {
    inline static bool read(std::istream& is)
    {
        is >> std::boolalpha;
        bool x;
        is >> x;
        return x;
    }
};

template<typename T>
inline T read(std::istream& is)
{
    T x = istream_traits<T>::read(is);
    return x;
}

... and then replace T result; iss >> result; with return read<T>(iss);.

(*) - Yes, I know anything with iostreams is probably not fast to begin with.

Just going to review your original code and not the edit.

inline std::istringstream& get_istringstream(){
    static thread_local std::istringstream stream;
    stream.str("");
    return stream;
}

template <typename T>
inline T from_string(const std::string& s){
    auto& iss(get_istringstream());
    iss.str(s);
    T result;
    iss >> result;
    return result;
}

As Jerry Coffin mentioned, you are basically reinventing boost::lexical_cast.  I wasn't sure if I wanted to review this or flag it as broken code, but it technically compiles and it technically works for one conversion (perhaps a discussion for meta-cr).  Test your code thoroughly.

  Are there other considerations I've overlooked, performance-wise (*) or usability-wise?

You don't handle errors.  Consider someone doing this:

auto x1 = from_string<int>("Not an int");

What is the value of x1?

    T result;      // uninitialized variable.
    iss >> result; // operator>> fails silently, sets failbit
    return result; // return uninitialized variable.

The result is undefined behavior since we are returning an uninitialized value that was never assigned to.  Remember to check that the reading of a value was successful.  Notify the callee whenever that an error, like invalid argument or out of range, has occurred.

There are different error handling strategies today.  The common ones are exceptions, std::system_error, and Alexandrescu's expected<T, E> (currently being proposed for standardization).

Let's say someone does something common, like use your function more than once.

auto x1 = from_string<int>("42");
auto x2 = from_string<int>("43");

The first line returns 42, that is fine.  The second line has an issue.

    T result;      // uninitialized variable.
    iss >> result; // eofbit enabled from previous stream consumption
    return result; // return uninitialized variable.

Undefined behavior again.  The original code had

std::istringstream iss;

On construction, isss internals were initialized (data and error state).  In your refactored code, you wanted to mimic a newly constructed object

inline std::istringstream& get_istringstream(){
    static thread_local std::istringstream stream;
    stream.str("");
    return stream;
}

    auto& iss(get_istringstream());

Turns out your refactored code wasn't equivalent.  You forgot to clear the error state.

Consider the following code:

auto x1 = from_string<double>("42,004");

What is the value of x1?  Conversion of arithmetic types using separators is dependent on the locale of the stream.  The possible results are

forty-two thousand and four,
forty two and four one-thousandths, or
forty-two.

If you are fine with this behaviour, document it.  If you want to ensure you always use the behavior of a specific locale by default, consider imbuing before reading.

  Perhaps I should mention my motivation here is partly how I found it strange that there's no std::from_string().

<std::string> provides functions to convert to arithmetic types

Reals - std::stod, std::stold, std::stof
Signed integrals - std::stoi, std::stol, std::stoll, and
Unsigned integrals std::stoul, std::stoull.

You could also consider copying the naming scheme from these and have string_to<T>.

For general purpose conversions, the ones that immediately come to mind are  boost::lexical_cast (stream-based) and boost::spirit (policy-based).  Boost also has boost::convert, an adapter library built to work with various conversion utilities (strtol, boost::spirit, boost::lexical_cast, and C++-Streams).


Breaking a line segment into two where it encounters a rectangle I have developed a function that takes a list of line segments and a rectangle that breaks the line segment into two where it encounters the rectangle. Here is a picture demonstration of what it does:

And here is a full demo code representation here:

var ctx = document.querySelector("canvas").getContext("2d");

function drawSegments(segments) {
  for (let seg of segments) {
    ctx.beginPath();
    ctx.moveTo(seg.x1, seg.y1);
    ctx.lineTo(seg.x2, seg.y2);
    ctx.stroke();
  }
}

function drawRectangles(rectangles) {
  for (let r of rectangles) {
    ctx.beginPath();
    ctx.rect(r.x, r.y, r.w, r.h);
    ctx.stroke();
  }
}

// Returns the point (x, y) where two line segments intersect
function getLineIntersection(p0_x, p0_y, p1_x, p1_y, p2_x, p2_y, p3_x, p3_y) {
  let s1_x = p1_x - p0_x;
  let s1_y = p1_y - p0_y;
  let s2_x = p3_x - p2_x;
  let s2_y = p3_y - p2_y;

  let s = (-s1_y * (p0_x - p2_x) + s1_x * (p0_y - p2_y)) / (-s2_x * s1_y + s1_x * s2_y);
  let t = ( s2_x * (p0_y - p2_y) - s2_y * (p0_x - p2_x)) / (-s2_x * s1_y + s1_x * s2_y);

  if (s >= 0 && s <= 1 && t >= 0 && t <= 1) {
    return { x: p0_x + (t * s1_x), y: p0_y + (t * s1_y) };
  }

  return false;
}

function breakSegments(segments, rect) {
  let rLeft = rect.x;
  let rTop = rect.y;
  let rRight = rect.x + rect.w;
  let rBot = rect.y + rect.h;

  for (let i = 0; i < segments.length; i++) {
    let s = segments[i];

    let nIntersection = getLineIntersection(s.x1, s.y1, s.x2, s.y2, rLeft, rTop, rRight, rTop);
    let wIntersection = getLineIntersection(s.x1, s.y1, s.x2, s.y2, rLeft, rTop, rLeft, rBot);
    let eIntersection = getLineIntersection(s.x1, s.y1, s.x2, s.y2, rRight, rTop, rRight, rBot);
    let sIntersection = getLineIntersection(s.x1, s.y1, s.x2, s.y2, rLeft, rBot, rRight, rBot);

    var closeIntersection, farIntersection;

    if (nIntersection && sIntersection) {
      let nXDelta = nIntersection.x - s.x1;
      let nYDelta = nIntersection.y - s.y1;

      let sXDelta = sIntersection.x - s.x1;
      let sYDelta = sIntersection.y - s.y1;

      if (nXDelta * nXDelta + nYDelta * nYDelta < sXDelta * sXDelta + sYDelta * sYDelta) {
        closeIntersection = nIntersection;
        farIntersection = sIntersection;
      } else {
        closeIntersection = sIntersection;
        farIntersection = nIntersection;
      }
    } else if (wIntersection && eIntersection) {
      let wXDelta = wIntersection.x - s.x1;
      let wYDelta = wIntersection.y - s.y1;

      let eXDelta = eIntersection.x - s.x1;
      let eYDelta = eIntersection.y - s.y1;

      if (wXDelta * wXDelta + wYDelta * wYDelta < eXDelta * eXDelta + eYDelta * eYDelta) {
        closeIntersection = wIntersection;
        farIntersection = eIntersection;
      } else {
        closeIntersection = eIntersection;
        farIntersection = wIntersection;
      }
    } else {
      continue;
    }

    segments.splice(i + 1, 0, { x1: farIntersection.x, y1: farIntersection.y, x2: s.x2, y2: s.y2 });

    s.x2 = closeIntersection.x;
    s.y2 = closeIntersection.y;

    break;
  }
}

let lineSegments = [
  { x1: 0, y1: 50, x2: 250, y2: 50 },
  { x1: 250, y1: 50, x2: 250, y2: 250 },
  { x1: 250, y1: 250, x2: 100, y2: 250 },
  { x1: 100, y1: 250, x2: 80, y2: 75 },
  { x1: 400, y1: 550, x2: 525, y2: 550 },
  { x1: 525, y1: 550, x2: 550, y2: 350 },
  { x1: 550, y1: 350, x2: 300, y2: 350 },
];

let rects = [
  { x: 200, y: 240, w: 16, h: 16 },
  { x: 500, y: 340, w: 16, h: 16 }
]

breakSegments(lineSegments, rects[0]);
breakSegments(lineSegments, rects[1]);

drawSegments(lineSegments);
drawRectangles(rects);
<canvas width="800" height="800">

As you can see it already works. What I don't like is how complex it is. I feel like this should be able to be accomplished with less code. Specifically, breakSegments is very verbose in how it determines which intersection is closer, as I use the distance formula to determine this. Is there no better way of accomplishing this?

General points

Use const when a variable does not change.

Avoid using underscore in JavaScript names

Making names too long makes code hard to read, especial when the relevant part of the name is tiny. eg nIntersection, eIntersection, sIntersection, wIntersection when in busy lines the n,e,w,s can easily be overlooked.

The function getLineIntersection returns a point or false. Rather than return a point return the value t.
This value represents the unit distance along the line of the intersection. It is easier to work with this value (finding closest point to start of line). See example

In the same function you calculate the cross product of the two lines as vectors. There is room for improvement.

You calculate this value twice. You need do it only once.

When the line are parallel the cross of the vectors will be zero. Lines will not intercept when parallel. Also it means you will be dividing by zero.

You calculate the unit distance along each line then you check if the intercept point is on both lines. You can improve the performance by checking each line as you go. No need to find the unit distance on the second line if it does not cross the first.

Rendering
When drawing paths that all have the same style move the ctx.beginPath outside the loop. ctx.beginPath forces a GPU state change and on many systems this can slow the whole device down, not just your page. (see Rewrite)
Vector Math
Consider creating or using a vector library to reduce long messy lines like let t = ( s2_x * (p0_y - p2_y) - s2_y * (p0_x - p2_x)) / (-s2_x * s1_y + s1_x * s2_y);
Expressions like that are very prone to typos and errors. Using a vector library gives readable abstraction to the operation. eg the above line may becomes t = s2.cross(line1.p1.sub(line2.p1)) / s1.cross(s2)
Even using a very basic library to define points and lines will help reduce the amount of code. see Rewrite.
Problems?
Hard to know if these are problems or if there are constraints that mean the following will not be of issue.

A line segment may start, end, or be completely inside a box. There is no code that take this into account

You stop clipping line segments on the first line clipped. This means that is more than one segment crosses the box the second etc will not get clipped.

Assuming that the code is working, then there is no need to clip when the line segment ends on the box's  edge. Thus the test if (s >= 0 && s <= 1 && t >= 0 && t <= 1) {  can be if (s > 0 && s < 1 && t > 0 && t < 1) {

You only clip lines is they cross the box, If lines  cross the corner of a box you do not clip it?

Rewrite
I have added 4 very basic objects, Point2, Vector2, Line2, and  Rectangle2 (2 stands for 2D) and use these to store and do the calculations.
getLineIntersection returns the unit distance along the line segment rather than a point. The unit distance is push to an array. If 2 units are in that array then the segment will be clipped. The units distances are sorted to get the first and second intercepts.
The behavior remains the same as your original code apart from clipping only crossing opposite sides of the box. The rewrite will clip if line crosses adjoining box edges. The data includes this as a 3rd box.

const ctx = canvas.getContext("2d");

const Point2 = (x = 0, y = 0) => ({x, y});
const Vector2 = Point2;
const Line2 = (p1, p2) => ({p1, p2});
const Rectangle2 = (x, y, w, h) => ({p1: Point2(x, y), size: Vector2(w, h)});

function drawSegments(segments) {
    ctx.beginPath();
    for (const seg of segments) {
        ctx.moveTo(seg.p1.x, seg.p1.y);
        ctx.lineTo(seg.p2.x, seg.p2.y);
    }
    ctx.stroke();
}

function drawRectangles(rectangles) {
    ctx.beginPath();
    for (const r of rectangles) {
        ctx.rect(r.p1.x, r.p1.y, r.size.x, r.size.y);
    }
    ctx.stroke();
}

function getLineIntersection(l1, l2) {
    const s1 = Vector2(l1.p2.x - l1.p1.x, l1.p2.y - l1.p1.y);
    const s2 = Vector2(l2.p2.x - l2.p1.x, l2.p2.y - l2.p1.y);
    const df = Vector2(l1.p1.x - l2.p1.x, l1.p1.y - l2.p1.y);
    const cross = s1.x * s2.y - s1.y * s2.x;
    if (cross !== 0) {
        const u = (s1.x * df.y - s1.y * df.x) / cross;
        if (u > 0 && u < 1) {
            const u = (s2.x * df.y - s2.y * df.x) / cross;
            if (u > 0 && u < 1) { return u }
        }
    }
    return false;
}

function breakSegments(segments, rect) {
    const r1 = rect.p1;
    const r2 = Point2(r1.x + rect.size.x, r1.y);
    const r3 = Point2(r2.x, r2.y + rect.size.y);
    const r4 = Point2(r1.x, r3.y);

    const boxLines = [
        Line2(r1, r2),
        Line2(r2, r3),
        Line2(r3, r4),
        Line2(r4, r1),
    ];
    const units = [];
    var i = 0;
    while (i < segments.length) {
        units.length = 0;
        const line = segments[i++];
        for (const bLine of boxLines) {
            const u = getLineIntersection(line, bLine);
            u && units.push(u);
        }

        if (units.length === 2) {
            units[0] > units[1] && ([units[0], units[1]] = [units[1], units[0]]);
            const [u1, u2] = units;
            const p1 = line.p1, p2 = line.p2;
            const vx = p2.x - p1.x;
            const vy = p2.y - p1.y;
            line.p2.x = line.p1.x + vx * u1;
            line.p2.y = line.p1.y + vy * u1;
            segments.splice(i - 1, 0, Line2(
                Point2(line.p1.x + vx * u2, line.p1.y + vy * u2),
                Point2(line.p1.x + vx, line.p1.y + vy)
            ));
            break;
        }
    }
}

const lineSegments = [
    Line2(Point2(10,   60),  Point2(500, 60)),
    Line2(Point2(100, 10), Point2(100, 120)),
    Line2(Point2(220, 170), Point2(300, 110)), // cross corner segment
    //Line2(Point2(256, 0), Point2(256, 120)),  // these lines do not get clipped
    //Line2(Point2(263, 120), Point2(263, 0)),
];

const rects = [
    Rectangle2(250, 50, 16, 16),
    Rectangle2(90, 80, 16, 16),
    Rectangle2(250, 140, 36, 36)  // cross corner box
];
breakSegments(lineSegments, rects[0]);
breakSegments(lineSegments, rects[1]);
breakSegments(lineSegments, rects[2]);
drawSegments(lineSegments);
drawRectangles(rects);
<canvas id="canvas" width="500" height="500"></canvas>


Are these try/catch blocks cluttering the code? The following program reads a stream and creates a two dimensional std::vector.

I'm trying to find an elegant way to handle the errors and recover from them as much as possible while still reporting the problems encountered.

It feels as though my nice clean functions are getting more and more cluttered as I add the error handling.

Is this the best way?

#include <vector>
#include <iostream>
#include <sstream>
#include <string>
#include <iomanip>
#include <stdexcept>

namespace {

    using Grid_row = std::vector<int>;
    using Grid = std::vector<Grid_row>;

    class Partial_value : public std::runtime_error {
    public:
        Partial_value(std::string text, int val) : std::runtime_error{text}, val{val} {}
        int val{};
    };

    int convert(const std::string& text)
    {
        int val{};
        try {
            val = std::stoi(text);
        }
        catch(const std::exception& e) {
            throw Partial_value{"Due to error: \"" + std::string{e.what()} +"\"; Could not convert " + text + " to integer. Value was converted to 0.", val};
        }
        std::ostringstream oss{};
        oss << val;
        if(oss.str() != text)
            throw Partial_value{"Could not fully convert " + text + " to integer. Value was converted to " + oss.str() + ".", val};
        return val;
    }

    std::istream& operator >> (std::istream& is, Grid_row& grid_row)
    {
        std::string term{};
        while(is >> term) {
            int val{};
            try {
                val = convert(term);
            }
            catch(const Partial_value& e) {
                val = e.val;
                std::cerr << e.what() << '\n';
            }
            grid_row.push_back(val);
        }
        return is;
    }

    std::istream& operator >> (std::istream& is, Grid& grid)
    {
        std::string line{};
        while(std::getline(is, line)) {
            std::istringstream is_line{line};
            Grid_row grid_row{};
            is_line >> grid_row;
            grid.push_back(grid_row);
        }
        return is;
    }

    std::ostream& operator << (std::ostream& os, const Grid& grid)
    {
        for(const auto& i : grid) {
            for(const auto& j : i) {
                os << std::setw(5) << j;
            }
            os << '\n';
        }
        return os;
    }
}

auto input_text{
R"(3 -1 -3 -1 -4
40 7.5 3 1 1
10 -2 -Z 1 1
10 0 1 0 -1
)"};

int main() {
    std::istringstream is{input_text};
    Grid grid{};
    is >> grid;
    std::cout << grid;
    return 0;
}

Produces:

Could not fully convert 7.5 to integer. Value was converted to 7.
Due to error: "invalid stoi argument"; Could not convert -Z to integer. Value was converted to 0.
    3   -1   -3   -1   -4
   40    7    3    1    1
   10   -2    0    1    1
   10    0    1    0   -1

Your convert function should be pure.

convert should be doing one thing and one thing only, and that is to convert the given string to int. All those checks you do are not useful because they can easily be performed in the function that calls convert.

int convert(const std::string& text) {
    return std::stoi(text);
}

Your convert function is not needed.

Seeing as after changing it to a pure function, it is now acting as a wrapper to another function call, you don't need it anymore, and it can now be replaced by the function it calls, std::stoi.

Move convert logic elsewhere

Move all the logic from within the convert method to your overloaded function, std::istream& operator >> (std::istream& is, Grid_row& grid_row).

std::istream& operator >> (std::istream& is, Grid_row& grid_row) {
    std::size_t fin;
    int val;

    for (std::string term; is >> term; ) {
        try {
            val = std::stoi(term, &fin);
            if (fin != term.size()) {
                std::cerr << "Could not fully convert " + text + " to integer. Value was converted to " + text.substr(0, fin) + "." << '\n';
            }
        } catch(const std::exception& e) {
            std::cerr << "Due to error: \"" + std::string{e.what()} +"\"; Could not convert " + text + " to integer. Value was converted to 0." << '\n';
        }

        grid_row.push_back(val);

    }
    return is;
}

Now all you have is one try-catch, which is really all that is needed


Prevent multiple ifelse statements in php I'm trying to figure out a way to make my code more readable and maintainable. While rewriting some ugly spaghetti code, I stumbled across this:

if($header !== null && $fieldValue === null)
{
    $errorMessage =  $message . ': row ' . $row . ', header "' . $header . "\"\n";
}
elseif($header === null && $fieldValue !== null)
{
    $errorMessage =  $message . ': row ' . $row . ', field value "' . $fieldValue . "\"\n";
}
elseif($header === null && $fieldValue === null && $row === null)
{
    $errorMessage =  $message . "\n";
}
elseif($header !== null && $fieldValue !== null)
{
    $errorMessage =  $message . ': row ' . $row . ', header "' . $header . '", value "' . $fieldValue . "\"\n";
}

It must be possible to make this more efficient, but how? Do I use a switch statement? Please enlighten me.

Try something like this

<?php

function buildErrorMessage($message, $row = null, $header = null, $fieldValue = null) {

    if ($row) {
        $message .=  ": row {$row}";
    }

    if ($header) {
        $message .=  ", header {$header}";
    }

    if ($fieldValue) {
        $message .=  ", value \"{$fieldValue}\"";
    }

    return $message;
}

// Tests
echo buildErrorMessage('one');
echo buildErrorMessage('two', 2);
echo buildErrorMessage('three', 3, 'header');
echo buildErrorMessage('four', 4, 'header', 'value');
echo buildErrorMessage('five', null, null, 'value');


Calculating reactions to load on a beam I am new to Ruby and would like some help with re-factoring my code.

class Beam
def initialize
    puts "Please specify span of beam"
    @span = gets.chomp.to_f
    puts "How many number of point loads do you want?"
    @pointLoadNos = gets.chomp.to_i
    @pointLoad = Array.new(@pointLoadNos)
    @pointLoadDistFromLeft = Array.new(@pointLoadNos)
    @pointLoadDistFromRight = Array.new(@pointLoadNos)
    @magnitude = Array.new(@pointLoadNos)
    @reaction = Array.new(2,0)
end
def setValues
    count = 0
    while count < @pointLoadNos
        puts "At what dist should point load #{count+1} be placed from left?"
        @pointLoadDistFromLeft[count] = gets.chomp.to_f
        if @pointLoadDistFromLeft[count] > @span
            puts "Dist of Point Load #{count+1} From Left should be less than span length of beam"
        else
            puts "Magnitude of point load #{count+1} should be?"
            @magnitude[count] = gets.chomp.to_f
            @pointLoadDistFromRight[count] = (@span - @pointLoadDistFromLeft[count])
            count += 1
        end
    end
end
def calReactions
    i = 0
    while i < @pointLoadNos
        @reaction[0] += (@pointLoadDistFromLeft[i]*@magnitude[i])/@span
        @reaction[1] += (@pointLoadDistFromRight[i]*@magnitude[i])/@span
        i += 1
    end
    puts "Reaction at Left: #{@reaction[0]}"
    puts "Reaction at Left: #{@reaction[1]}"
end
end

beam = Beam.new
beam.setValues
beam.calReactions

First of all, you should not get user input from inside your Beam class. This ties it to the console and makes automated testing more difficult. A layered architecture is more suitable :

# The Beam class is only concerned by business logic.
# Notice how the logic and workflow are clear and readable,
# and how easy you it would be to write tests for it.
#
class Beam

  attr_reader :span

  def initialize( span )
    @span        = span.to_f
    @load_points = {}
  end

  def add_load_point( distance, magnitude )
    distance, magnitude = distance.to_f, magnitude.to_f
    if distance < 0 || distance > span || @load_points[distance]
      raise ArgumentError, 'invalid load point'
    end
    @load_points[distance] = magnitude
  end

  def reactions
    @load_points.inject( left: 0, right: 0 ) do |hash, (distance, magnitude)|
      hash[:left]  += distance          * magnitude / span
      hash[:right] += (span - distance) * magnitude / span
      hash
    end
  end

end

# this module is an interface between your class and the user
# via the console. Notice that this module (could be a class,
# does not matter) is only concerned about asking questions,
# adapting user input to feed the Beam class interface, and
# formatting output.
#
module BeamConsole

  # here we chose to move all questions to the user to methods,
  # and name these methods with an exclamation mark at the end
  #
  def self.invoke
    beam = Beam.new( span? )
    begin
      beam.add_load_point( distance?, magnitude? )
    rescue ArgumentError => e
      puts "Invalid parameters for load point. Please retry." ; retry
    end while more?
    format_reactions( beam.reactions )
  end

  private #===================================================================

  # adapted from @nakilon's answer, tail-recursive variant
  def self.prompt( msg )
    output = gets.strip!
    return output unless output.empty?
    prompt( msg )
  end

  def self.span?
    prompt( 'Please specify span of beam' ).to_f
  end

  def self.more?
    answer = prompt( 'Do you want to add points ? (y/n)' )
    answer = prompt( 'please answer by y or n' ) unless answer =~ /^[y|n]$/
    answer == 'y'
  end

  def self.distance?
    prompt( 'At what distance should the point be placed from left?' ).to_f
  end

  def self.magnitude?
    prompt( 'Magnitude should be?' ).to_f
  end

  def self.format_reactions( reactions )
    puts "Reaction on the left is  : %.4f" % reactions[:left]
    puts "Reaction on the right is : %.4f" % reactions[:right]
  end
end

This may seem overkill, but separating concerns is the core of OOP. With this approach, you'll end up with a Beam class devoid of any responsability concerning user interaction, and that's a good thing : you want your class to perform business logic before anything else.

This way, your code will be more readable (no logic riddled with prompts), so more maintainable, but also more reusable. Imagine that one day you want to allow users to perform calculations from a web page : you could not reuse your Beam class if it is tied to the console, but you could if you separated responsibilities.


Caching side effects Is this a proper way to cache side effects (occurring on job.tryLoad)?

case class DFStorage(private var cache: Map[String, DataFrame] = Map()) {
  def tryLoad(job: Job): Kleisli[IO, MakeContext, \/[List[String], Unit]] = {
    if(!cache.contains(job.id)) {
      job.tryLoad.map(_.map(df => add(job, df)))
    } else {
      IO(().right[List[String]]).liftKleisli
    }
  }
  def add(job: Job, df: DataFrame): IO[Unit] = {
    IO(cache += (job.id -> df))
  }
  def toMap = IO(cache)
}

Looks good to me.

Although, having cache as a simple Map might lay claim to more memory that you like. Associating an LRU with it would allow each addition to evict an ancient job's cached dataframe.


Output int from string input This takes a string input like 1,2,3,4,5, splits it using , delimiter, converts each char to int and output it in a single line like this : 12345

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

int* createIntArray(char* numbersInString, int* array_size);

int* createIntArray(char* numbersInString, int* array_size)
{
    char* token = strtok(numbersInString,",");
    int* numbersArray = malloc(sizeof(int)*100);
    int i = 0;

    while(token != NULL)
    {
        numbersArray[i] = (int)*token -'0';
        token = strtok(NULL,",");
        i++;
    }

    *array_size = i;
    return numbersArray;
}

int main(){
    char* stringNumbers = malloc(sizeof(char)*100);
    int* intArray;
    int arrayLength = 0;
    printf("Enter numbers\n");
    scanf("%s",stringNumbers);
    intArray = createIntArray(stringNumbers,&arrayLength);

    int i;
    for(i = 0; i < arrayLength;i++)
    {
        printf("%d",intArray[i]);
    }

    free(stringNumbers);
    free(intArray);
    return 0;
}

I know the code is super useless, but I'm starting to learn C and I wanted to see if my code was alright and if it complies to C coding standards.

Since you have main() below the other function, you don't need the prototype for that function.  It's only needed if main() is defined before a function that it calls.
There's no need to declare a variable and then assign to it:

int* intArray;
// ...
intArray = createIntArray(stringNumbers,&arrayLength);

Variables should just be initialized in the closest scope possible.  This will ease maintenance as you won't have to search for the declared variable elsewhere, such as if it's no longer in use.

int* intArray = createIntArray(stringNumbers,&arrayLength);

Instead of incrementing i in createIntArray(), why not just increment array_size?

Plus, i is more commonly used as a for loop counter, which isn't the case here.  If someone were to take a glance at that while loop without further context, they may think that it should be a for loop instead.  By making this change, you'll remove that possible misconception and better communicate your exact intentions.


Taking n lines from a file which contains m lines, repeating the file lazily if necessary The desire for such a function came out of the necessity of generating a "source" of Lorem ipsum by repeating over and over the same 50 or something paragraphs of the text.
Actually I am curious to know how to do it in C++ (I guess boost::hana has something to offer in this respect), and I have asked a question on StackOverflow about it aleady.
However, to get a better understanding of the functional approach, here's my working solution in Haskell,
import System.IO (readFile)
main :: IO ()
main = (\x -> readFile "file" >>= putStrLn . unlines . take x . concat . repeat . lines)
       =<< return . read
       =<< getLine

where

I'm not sure about the return . read type being the best choice there where it is (one reason being that it doesn't crash gracefully if main is fed with a non-numeric input), and
I haven't written the lambda totally in point-free because I think it would be less readable.
Most importantly, I'm not sure this works lazily in all its parts, but I'd really like to understand this bit.

The program above assumes the current directory contains a text file named file, which can contain something like this,
line1
line2
line3

Given this, one can compile and run obtaining this, for instance:
ghc --make -dynamic program.hs && ./program <<< 7
line1
line2
line3
line1
line2
line3
line1

Any suggestions?

Even though module Main is implied when no header is given, I'd write it.
return . read =<< getLine = read <$> getLine = fmap read getLine is a worse version of readLn. read <$> getLine is basically return (error msg) when the parse fails. This passes the bottoming value into the "pure" part of your code. readLn badstr crashes at a well defined time—upon its execution—and if you actually wanted to handle the error you'd use Text.Read.readMaybe or readEither.
concat . repeat is cycle. Gets your point across better, doesn't it?
putStrLn . unlines is a bug. It causes an extra newline in the input: unlines creates a trailing newline and putStrLn creates another. Use putStr.
I don't hate the "expression" style here, but once we replace the =<< return . read =<< getLine with =<< readLine you may as well bring it to the front and use >>=, and at that point let's just use a do.
x? Really?

So:
module Main (main) where -- not my style of spacing; but fine
import System.IO (readFile)

main = do
    wanted <- readLn
    contents <- readFile "file"
    putStr $ unlines $ take wanted $ cycle $ lines contents

Though I'd also take
main = do
    wanted <- readLn
    putStr . unlines . take wanted . cycle . lines =<< readFile "file"

Or, if you're attached
main = readLn >>= (\wanted -> putStr . unlines . take wanted . cycle . lines =<< readFile "file")

As to your concern about laziness; this is perfectly fine. readFile is lazy, and so is lines. cycle is actually better than concat . repeat; the latter keeps allocating (:)-cells as long as take consumes them, while cycle will simply connect a pointer that cycles back to the beginning of the output list, which should make it faster. Also, it errors instead of potentially looping forever on an empty input. take is of course lazy, and we've eliminated the excess laziness that would cause any parse errors to be triggered in it. Passing an infinite input (am on a *nix, so I used yes(1) and mkfifo(1)) into this program doesn't break it, which is a good litmus test for laziness.
Now, as to the functionality of this program: the wanted number should really be an argument, and so should the file (which should be optional and default to stdin). That's done with getArgs. This is complicated enough to move into new definitions. Let's get that readMaybe going, too. (Hoogle the new names to find the imports.)
dieWithUsage :: IO a
dieWithUsage = do
    name <- getProgName
    die $ "Usage: " ++ name ++ " lines [file]"

parseArgs :: [String] -> IO (Int, Handle)
parseArgs [] = dieWithUsage
parseArgs (wanted : rest) = (,) <$> getWanted <*> getHandle
    where getWanted = maybe dieWithUsage return $ readMaybe wanted
          getHandle = case rest of
              [] -> return stdin
              [path] -> openFile path ReadMode
              _ -> dieWithUsage

leaving
main = do
    (wanted, input) <- parseArgs =<< getArgs
    putStr . unlines . take wanted . cycle . lines =<< hGetContents input


Rails API for communication between two Rails app where the former app posts data to the latter and the latter return a response The Controller code for the first app to post data to another app

 module Api
 module V1
 class OrderApiController < ApiController
   def send_invoice_data
        response = Hash.new
        result = Hash.new
        debugger
        order = Order.includes(:status, :user, payment:
         [:status]).where(txnid: params[:txnid]).first
        if order
            if (['Notary', 'Attestation','Franking'].include?
        order.service.name)
                no_of_copies = ((order.answers.where(question_id:
             [37,15]).length > 0) ? order.answers.where(question_id:
            [37,15]).first.body : 0).to_i
            else
                no_of_copies = ((order.answers.where(question_id:
                [37,15]).length > 0) ? order.answers.where(question_id:
                [37,15]).first.body : 0).to_i + 1
            end
             response.merge! ApiStatusList::OK
             response =
                 HTTParty.post('http://localhost:3001/api/v0/generate_invoice?
                 key=docket', :body => { "order" => {
                      "id" => order.id,
                      "txnid" => order.txnid,
                      "service_name" => order.service.name,
                      "payment_date" => order.payment ?
                       order.payment.created_at.strftime('%d/%m/%Y') :
                       order.created_at.strftime('%d/%m/%Y'),
                                :
                                :
                      "delivery_amount" => order.delivery_amount || '',
                      "no_of_copies" => no_of_copies}}.to_json, :headers => {
                     'Content-Type' => 'application/json'})
        else
            response.merge! ApiStatusList::INVALID_REQUEST
        end
        render :json => response
    end
   end
   end
   end

The second controller api code accepts data from the former app, creates and generates the invoice pdf using wicked_pdf, uploads it to aws and returns the link of the stored pdf as response to latter app

 module Api
module V0
    class InvoiceApiController < ApiController

        def order_invoice
            response = Hash.new
            result = Hash.new
            debugger
            if params[:order] && params[:order][:txnid]
                @order = params[:order]
                @order['stamp_amount'] = params[:order][:stamp_amount] || ''
                @order['txnid'] = params[:order][:txnid]
                @order['service_name'] = params[:order][:service_name] || ''
                @order['payment_date'] = params[:order][:payment_date]
                @order['payment_mode'] = params[:order][:payment_mode] || ''
                    :
                    :
                @order['discount'] =  params[:order][:discount] || ''
                @no_of_copies = params[:order][:no_of_copies]
                pdf = WickedPdf.new.pdf_from_string(
                render_to_string(template:
                'invoices/generate_invoice.pdf.erb', filename: params[:order]
                 [:txnid] + ".pdf" ,
                                type: 'application/pdf', disposition:
                 'attachment', print_media_type: true))
                save_path = Rails.root.join('pdfs', @order['txnid'] + ".pdf")
                File.open(save_path, 'wb') do |file|
                  file << pdf
                  filename = @order['txnid'] + ".pdf"
                end
               file_name =  @order['txnid'] + ".pdf"
               upload = Invoice.upload(save_path, file_name)
               response['result'] = upload
               response.merge! ApiStatusList::OK
            else
                response.merge! ApiStatusList::INVALID_REQUEST
            end
            render :json => response
        end

I will try to approach this from top to bottom, and mention things as I notice them.

(1) First obvious one is indentation, as mentioned in the other answer. The standard is 2 spaces. Nested modules and classes should be indented:

module Api
  module V1
    class OrderApiController < ApiController
    end
  end
end

(2) Extract logic into object(s) or other methods. You have started what is and will increasingly be a difficult piece of code to maintain. If you find yourself dumping everything into a single method, and that method grows beyond 4 or 5 lines, you could probably clean it up by using multiple methods and/or objects.

That's all well and good, but what does this look like? This means your send_invoice_data should look something like this:

def send_invoice_data
  render json: generate_invoice
end

def generate_invoice
  @generate_invoice ||= Api::GenerateInvoice.post(order)
end

module Api
  class GenerateInvoice
    def initialize(order)
      @order = order
    end

    def self.post(order)
      new(order).post
    end

    def post
      HTTPParty.post(url, body: data)
    end

    private

    def url
      'http://localhost:3001/api/v0/generate_invoice?key=docket'
    end

    def data
      {
        id: @order.id,
        txnid: 'etc'
      }
    end
  end
end

It is important you understand what we have done above, and the benefits that gives you. For one, it has instantly cleaned up the controller method and now you can solely worry about dealing with the response for the action.

The GenerateInvoice object has allowed us to not just extract all of that messy logic, but given us the ability to further break it down into small, descriptive methods. This will greatly improve the readability and maintainability of it. You also have the added benefit of being able to use this class elsewhere, should you need to.

Let's delve into the actual method now.

The first thing you are doing is initializing a couple of variables. response is either being assigned later (response = HTTParty) or you are using merge (response.merge! etc). You can remove this variable initialization and simply assign it later. Remove your merge and use response = ApiStatusList::INVALID_REQUEST.

You should remove debugger before you commit the code.

The Order.includes part should be extracted into a method:

def order
  @order ||= Order.includes(:status, :user, payment: [:status])
                  .find_by(txnid: params[:txnid])
end

You can also extract the scope into Order itself:

class Order < ActiveRecord::Base
  scope :eager, -> { includes(:status, :user, payment: [:status]) }
end

..and use..

def order
  @order ||= Order.eager.find_by(txnid: params[:txnid])
end

Now this might appear to be a bit extreme or contrived, but it is important to notice what I'm doing as I refactor this code. What is informing my main design decisions is the idea of extracting things into smaller, descriptive pieces. Just following that path alone is vastly improving the design of the code. Keep that in mind as you write and refactor code.

Another great place for extraction is checking whether and order is of a certain service type. You could extract this directly into your Order class again under what we call a predicate method:

class Order
  def service_i_want?
    services.include?(service.name)
  end

  def services
    %w(Notary Attestation Franking)
  end
end

This means your send_invoice_data will now look like:

def send_invoice_data
  if order
    if order.services_i_want?
      no_of_copies = # etc
    else
      no_of_copies = # etc
    end

    # HTTParty.post etc
  else
    ..
  end
end

Again, lots of extraction going on. Either into methods, or other classes.

Looking at the above we can see yet another opportunity for extraction - the no_of_copies. Instead of initializing a variable, extract it into it's own method..

def no_of_copies
  if order.services_i_want?
    ((order.answers.where(question_id:
             [37,15]).length > 0) ? order.answers.where(question_id:
             [37,15]).first.body : 0).to_i
  else
    ((order.answers.where(question_id:
             [37,15]).length > 0) ? order.answers.where(question_id:
             [37,15]).first.body : 0).to_i + 1
  end
end

I think you know where I'm going next ;) Extracting this into its own method has clearly shown us these two branches have essentially the same code. In the effort to DRY things up we should extract further.

First, let's breakdown what you are doing here.

If you have answers that have the question id of 37 or 15, then fetch body of the first answer. If no answers, return 0. Convert result to_i.

To better express this, you should, guess.. extract it into smaller parts! The logic itself probably belongs in the Order class. So first thing would be to extract it there:

class Order
  def number_of_copies
    # etc
  end
end

Because you are now in the Order context, you can better extract the other parts. We want answers with question id of 37 or 15 first..

def copied_answers
  answers.where(question_id: [37, 15])
end

We can now reduce our number_of_copies method to this:

def number_of_copies
  ((copied_answers.length > 0) ? copied_answers.first.body : 0).to_i
end

MUCH easier to think about! Looking at your logic from this viewpoint, we can now easily rewrite it to give us what we want in a much more elegant way:

def number_of_copies
  if copied_answers.any?
    copied_answers.first.body.to_i
  else
    0
  end
end

Or if you like your ternaries:

def number_of_copies
  copied_answers.any? ? copied_answers.first.body.to_i : 0
end

Let's go back to our copied_answers method for a second.

def copied_answers
  answers.where(question_id: [37, 15])
end

As the other answer mentioned, you should refrain from using 'magic numbers'. 37 and 15 above are magic because they give me no idea of what they represent. Extract them into methods or constants or whatever. Just hide them behind something descriptive so I can use this:

def copied_answers
  answers.where(question_id: [MY_QUESTION, ANOTHER_QUESTION])
  # or
  answers.where(question_id: question_ids)
end

Describe what the questions are so somebody can understand why these question ids are connected to 'copied' answers.

We also have another piece of logic to address. If the order service name is not in that services list above (services_i_want), then we increment the number of copies. I'm open to suggestions, but my first attempt at that would be:

def number_of_copies
  copies = 0

  if copied_answers.any?
    copies = copied.answers.first.body.to_i
  end

  copies += 1 if services_i_want?

  copies
end

That can no doubt be improved, but I'm writing that just as a way to match the current logic. It is, regardless, a vast improvement of what we had previously.

Let's return to your send_invoice_data method.

Because we have extracted the logic that determines the number of copies into number_of_copies on the Order itself, our send_invoice_data now looks like:

def send_invoice_data
  if order
    response = HTTParty.post('http://localhost:3001/api/v0/generate_invoice?key=docket',
      :body => {
        "order" => {
        "id" => order.id,
        "txnid" => order.txnid,
        "service_name" => order.service.name,
        "payment_date" => order.payment ?
          order.payment.created_at.strftime('%d/%m/%Y') :
          order.created_at.strftime('%d/%m/%Y'),
        "delivery_amount" => order.delivery_amount || '',
        "no_of_copies" => order.number_of_copies}}.to_json,
        :headers => { 'Content-Type' => 'application/json' }
      }
    )
  else
    response = ApiStatusList::INVALID_REQUEST
  end

  render json: response
end

Again, much easier to see what the hell is going on here now. If we have an order, we send that order to generate invoice endpoint and return the response. If no order found, we return INVALID_REQUEST.

Let's first extract the post into it's own method..

def generate_invoice
  # HTTParty.post(etc)
  # etc
  # etc
end

Notice this method from before? The natural progression would be to first extract into a method, then as that requires further breaking down, extract into other methods. If those methods begin to pollute the class we are in, extract it into its own object.

The end result is this:

module Api
  module V1
    class OrderApiController < ApiController
      def send_invoice_data
        if order
          render json: generate_invoice
        else
          render json: ApiStatus::INVALID_REQUEST
        end
      end

      def generate_invoice
        @generate_invoice ||= GenerateInvoice.post(order)
      end

      def order
        @order ||= Order.eager.find_by(txnid: params[:txnid])
      end
    end
  end
end

I will stop extracting things at this point and allow you to work through the rest by yourself. First place to start would be to create your GenerateInvoice class and throw all your logic into there. After that, just continue to extract things into small, descriptive methods, and, if necessary, other objects.

Use the advice in these answers and apply what you've learned to your second file. It's the same principles and if you follow them will lead to cleaner code.

A starting point might be the payment_date value on your post request body. Extract into Order and simply call order.payment_date.

I hope this has helped you somewhat. If you want more help, please follow the advice above and refactor your code, then post another question and I will give feedback there (do not update this question as will render my answer useless to other people).


Encryption algorithm using CTR mode I am using CTR mode (it is a cipher in itself) in this code. I just wanted to see what you thought about it before I finish it.

(Yes, I know that the plaintext length and key length must be highly specific. I will take care of that with padding later.)

I just wanted to know if you thought if this code was cryptographically secure and if I could speed it up a little.

class Encrypt
{
    public string CTR(string p, string key)
    {
        StringBuilder sb = new StringBuilder();

        for (uint c = 0; c < (p.Length / key.Length); c++ )
        {
            char[] kc = key.ToCharArray();
            char[] E = new char[key.Length];

            char[] cc = c.ToString().ToCharArray();
            int inc = 0;
            foreach (char ch in kc)
            {
                E[inc] = (char)(ch ^ cc[0]); // Something goes wrong here when there are 2 digits in uint c. (CodeReview plz help)
                inc++;
            }

            char[] pc = p.ToCharArray();

            for (int i = 0; i < E.Length; i++)
            {
                sb.Append((char)(pc[i] ^ E[i]));
            }
        }

        return sb.ToString();
    }
}

It looks like you are XORing a mildly modified key with the plaintext, then mildly modifying the key every 10 blocks.  As @SimonForsberg has noted, such simplistic schemes just are amateur-grade ciphers, and certainly not cryptographically secure.  (Cryptanalysis would be beyond the scope of Code Review, though.)

Only a few experts are capable of producing cryptographically secure algorithms — and even then, they submit their schemes to merciless peer review.  There's always someone out there who is smarter than you are.  Attacks only get better, never worse.  If you want a cryptographically secure scheme, don't roll your own crypto.


User creation and sign-in script I made code for signing in users and storing data about them. Everything is fully encrypted and secure also, it is meant to be imported into other code. I'm looking for ways to improve it and to fix any bugs people might find.

import pickle
import base64

class user():
    def __init__(self, username, password, user_data):
        self.username = username
        self.password = password
        self.user_data = user_data

def get_user():
    try:
        user_saves = open("users", 'rb')
        users = dict(pickle.load(user_saves))
        user_saves.close
        name, password, users = sign_in(users)
    except:
        name, password, users = new_user(True)

    name = base64.b64decode(name.decode('utf-8'))
    password = base64.b64decode(password.decode('utf-8'))
    return name, password, users

def new_user(is_first = False, users=None):
    print('Creating account... ')
    if users != None:
        loop = True
        while loop:
            name = input('What is your username: ')
            name = base64.b64encode(name.encode('utf-8'))
            if name in users:
                print('Already taken.')
            else:
                loop = False
                password = input('What is your password: ')
                password = base64.b64encode(password.encode('utf-8'))
    else:
        name = input('What is your username: ')
        name = base64.b64encode(name.encode('utf-8'))
        password = input('What is your password: ')
        password = base64.b64encode(password.encode('utf-8'))
    if is_first == True:
        user = {name:password}
        user_saves = open("users", 'wb')
        pickle.dump(user, user_saves)
        add_user_data('', password, True)
        return name, password, user
    else:
        add_user_data('', password)
        users.update({name:password})
        user_saves = open("users", 'wb')
        pickle.dump(users, user_saves)
        return name, password, users
    user_saves.close

def sign_in(users):
    nsi = True #not signed in
    while nsi == True:
        username = input('What is your username: ')
        username = base64.b64encode(username.encode('utf-8'))
        if username in users:
            password = input('What is your password: ')
            password = base64.b64encode(password.encode('utf-8'))
            if password == users.get(username):
                print('working')
                nsi = False
                return username, password
            else:
                print('Wrong password.')
        else:
            yn = y_or_n('y or n, are you a new user: ')
            if yn == True:
                nsi = False
                name, password = new_user(users=users)
                return name, password

def y_or_n(promt):
    ni = True #no input
    while ni:
        yn = input(promt).lower()
        if yn == 'y':
            return True
        elif yn == 'n':
            return False
        else:
            print('y or n')

def add_user_data(data, password, is_first=False):
    password = base64.b64encode(password.encode('utf-8'))
    data = base64.b64encode(data.encode('utf-8'))
    if is_first == True:
        user_data = {password:data}
        users_data = open("users_data", 'wb')
        pickle.dump(user_data, users_data)
    else:
        user_data.update({password:data})
        users_data = open("users_data", 'wb')
        pickle.dump(user_data, users_data)
    users_data.close

def get_user_data(password):
    password = base64.b64encode(password.encode('utf-8'))
    user_data = pickle.load('user_data')
    user_data = base64.b64decode(user_data.decode('utf-8'))
    return user_data

def delete_user(name, data, ):
    data.pop('name', None)

name, password, users = get_user()
data = get_user_data(password)
print(data)
user_stats = user(name, password, data)

Security

  Everything is fully encrypted and secure

Oh, really? I hope that you aren't confusing encoding with encryption. I don't see any encryption taking place.

This is a huge topic, so you have some reading to do - but the bar for 'secure password store' is significantly above where this program currently resides.

Class syntax

This:

class user():

should be

class User:

Also, you're currently using it as a struct with no methods. That should change - you should move some of your code to be methods on that class.

Break

This:

loop = True
while loop:

should be reworked. You can use a while True, and replace the loop = False with a break at the end of that if block.

File handles

Rather than explicitly closing your files, you should usually use them in a with block. Also, this:

user_saves.close

doesn't do what you think it does; in fact it does nothing. For the function call to occur, you need to add ().

Redundant else

This:

    return name, password, user
else:

doesn't need an else, because you've returned in the previous block.

Write a main method

...to pull this code out of global scope:

name, password, users = get_user()
data = get_user_data(password)
print(data)
user_stats = user(name, password, data)


Two FizzBuzz solutions Below are two solutions to the FizzBuzz problem in Python. Which one of these is more "Pythonic" and why is it more "Pythonic" than the other?

Solution One:

fizzbuzz = ''

start = int(input("Start Value:"))
end = int(input("End Value:"))

for i in range(start,end+1):
    if i%3 == 0:
        fizzbuzz += "fizz"
    if i%5 == 0:
        fizzbuzz += "buzz"
    if i%3 != 0 and i%5 != 0:
        fizzbuzz += str(i)

    fizzbuzz += ' '

print(fizzbuzz)

Solution Two:

fizzbuzz = []

start = int(input("Start Value:"))
end = int(input("End Value:"))

for i in range(start,end+1):
    entry = ''
    if i%3 == 0:
        entry += "fizz"
    if i%5 == 0:
        entry += "buzz"
    if i%3 != 0 and i%5 != 0:
        entry = i

    fizzbuzz.append(entry)

for i in fizzbuzz:
    print(i)

As has already been pointed out, creating a list is preferable as it avoids the concatenation of large strings. However neither of your solutions is the most pythonic solution possible:

Whenever you find yourself appending to a list inside a for-loop, it's a good idea to consider whether you could use a list comprehension instead. List comprehensions aren't only more pythonic, they're also usually faster.

In this case the body of the loop is a bit big to fit into a list comprehension, but that's easily fixed by refactoring it into its own function, which is almost always a good idea software design-wise. So your code becomes:

def int_to_fizzbuzz(i):
    entry = ''
    if i%3 == 0:
        entry += "fizz"
    if i%5 == 0:
        entry += "buzz"
    if i%3 != 0 and i%5 != 0:
        entry = i
    return entry

fizzbuzz = [int_to_fizzbuzz(i) for i in range(start, end+1)]

However, while we're at it we could just put the whole fizzbuzz logic into a function as well. The function can take start and end as its argument and return the list. This way the IO-logic, living outside the function, is completely separated from the fizzbuzz logic - also almost always a good idea design-wise.

And once we did that, we can put the IO code into a if __name__ == "__main__": block. This way your code can be run either as a script on the command line, which will execute the IO code, or loaded as a library from another python file without executing the IO code. So if you should ever feel the need to write a GUI or web interface for fizzbuzz, you can just load your fizzbuzz function from the file without changing a thing. Reusability for the win!

def fizzbuzz(start, end):
    def int_to_fizzbuzz(i):
        entry = ''
        if i%3 == 0:
            entry += "fizz"
        if i%5 == 0:
            entry += "buzz"
        if i%3 != 0 and i%5 != 0:
            entry = i
        return entry

    return [int_to_fizzbuzz(i) for i in range(start, end+1)]

if __name__ == "__main__":
    start = int(input("Start Value:"))
    end = int(input("End Value:"))
    for i in fizzbuzz(start, end):
        print(i)

(Note that I've made int_to_fizzbuzz an inner function here, as there's no reason you'd want to call it outside of the fizzbuzz function.)


Exercism: Class representation of a matrix in Python The task is to represent a Matrix as a class and to instantiate it from a string such as "1 2 3 4\n5 6 7 8\n9 8 7 6"
The class must provide methods for accessing individual rows and columns by index starting at 1.
While my solution is relatively short and more or less clear, I'm wondering about the ways to make it better.
One thing I'm not sure about is calling _get_rows() in different places inside the class, instead of assigning its result to class attribute.
Here is my code.
class Matrix:
    """Class representation of a matrix"""

    def __init__(self, matrix_string):
        self.matrix_string = matrix_string

    def _get_rows(self):
        """Produce a list of rows of the matrix."""

        rows = []
        for row_string in self.matrix_string.split('\n'):
            rows.append([int(char) for char in row_string.split()])

        return rows

    def _get_columns(self):
        """Produce a list of columns of the matrix."""

        rows = self._get_rows()

        return [list(row) for row in zip(*rows)]

    def row(self, index):
        """Produce i-th row of the matrix with first index being 1"""

        return self._get_rows()[index - 1]

    def column(self, index):
        """Produce i-th column of the matrix with first index being 1"""

        return self._get_columns()[index - 1]

And the unit tests provided by exercism.
import unittest

from matrix import Matrix

# Tests adapted from `problem-specifications//canonical-data.json` @ v1.3.0

class MatrixTest(unittest.TestCase):
    def test_extract_row_from_one_number_matrix(self):
        matrix = Matrix("1")
        self.assertEqual(matrix.row(1), [1])

    def test_can_extract_row(self):
        matrix = Matrix("1 2\n3 4")
        self.assertEqual(matrix.row(2), [3, 4])

    def test_extract_row_where_numbers_have_different_widths(self):
        matrix = Matrix("1 2\n10 20")
        self.assertEqual(matrix.row(2), [10, 20])

    def test_can_extract_row_from_non_square_matrix_with_no_corresponding_column(self):
        matrix = Matrix("1 2 3\n4 5 6\n7 8 9\n8 7 6")
        self.assertEqual(matrix.row(4), [8, 7, 6])

    def test_extract_column_from_one_number_matrix(self):
        matrix = Matrix("1")
        self.assertEqual(matrix.column(1), [1])

    def test_can_extract_column(self):
        matrix = Matrix("1 2 3\n4 5 6\n7 8 9")
        self.assertEqual(matrix.column(3), [3, 6, 9])

    def test_can_extract_column_from_non_square_matrix_with_no_corresponding_row(self):
        matrix = Matrix("1 2 3 4\n5 6 7 8\n9 8 7 6")
        self.assertEqual(matrix.column(4), [4, 8, 6])

    def test_extract_column_where_numbers_have_different_widths(self):
        matrix = Matrix("89 1903 3\n18 3 1\n9 4 800")
        self.assertEqual(matrix.column(2), [1903, 3, 4])

if __name__ == "__main__":
    unittest.main()
```

I would make a few suggestions, rather than just saving the input string it would be better, as you suggest, to save the output of the get_rows function in memory, so you don't have to recreate it every time.
Similarly when you want an individual row or column, it seems inefficient to gather all the rows, (and then all the columns) just to reject all but one of them.
Your comments say produces i-th (column/row)... but the variable is referred to as index
Minor points:
You don't need to save rows as a variable in _get_columns, you can just use [list(row) for row in zip(*self._get_rows())]
you could reduce _get_rows to one line : [[int(char) for char in row_string.split()] for row_string in self.matrix_string.split('\n')]
I'm assuming that you don't need to check if the input is invalid, because that isn't part of the testing suite. However you may want to do it anyway if you plan on using this class again.
In terms of style the code is good. The first comment under class Matrix doesn't really say anything and there are more newlines than are necessary, but other than that everything looks good.
You could try something like:
class Matrix:
    """Class representation of an integer matrix indexed from 1
    ...

    Attributes
    ----------
    rows: List[List(int)]
        A list of the rows of values in the matrix.

    Methods
    -------
    row(self, i)
        returns the i-th row.

    column(self, i)
        returns the i-th column.
    """

    def __init__(self, matrix_string):
        """Save a list of rows from the input string.
        - matrix_string: A space and new line seperated string of values
        in the matrix.
        """
        self.rows = self._get_rows(matrix_string)

    def _get_rows(self, matrix_string):
        """Produce a list of rows of the matrix.
        - matrix_string: A space and new line seperated string of values
        in the matrix.
        """
        rows = []
        for row_string in matrix_string.split('\n'):
            rows.append([int(char) for char in row_string.split()])
        return rows

    def _get_columns(self):
        """Produce a list of columns of the matrix."""
        return [list(row) for row in zip(*self.rows)]

    def row(self, i):
        """Produce i-th row of the matrix with first index being 1."""
        return self.rows[i - 1]

    def column(self, i):
        """Produce i-th column of the matrix with first index being 1."""
        return [row[i - 1] for row in self.rows]

or you could drop the _get_rows and change the __init__ to:
    def __init__(self, matrix_string):
        """Save a list of rows from the input string.
        - matrix_string: A space and new line seperated string of values
        in the matrix.
        """
        self.rows = [[int(x) for x in row.split()] for row in matrix_string.split("\n")]

In terms of performance:
using cProfile and the following script:
if __name__ == "__main__":
    for _ in range(100000):
        matrix = Matrix("1 2 3 4\n5 6 7 8\n9 8 7 6\n1000 2000 3000 4000")
        for i in range(1, 5):
            matrix.column(i)
            matrix.row(i)

Produces for the old matrix definition:
12900228 function calls (12900222 primitive calls) in 10.239 seconds
For the my definition:
1900228 function calls (1900222 primitive calls) in 1.359 seconds


Get a path between graph nodes using breadth-first search This code is meant to implement a Graph class which has a method that returns a path between 2 nodes using breadth-first search.

I'm doing this to improve my style and to improve my knowledge of fundamental algorithms/data structures for an upcoming coding interview.

from queue import PriorityQueue
from collections import deque
from copy import deepcopy

class Graph:
    def __init__(self):
        self.nodes = dict()

    def add_node(self, id):
        new_node = Node(id)
        self.nodes[id] = new_node
        return new_node

    def get_node(self, id):
        node = self.nodes[id] if id in self.nodes else None
        return node

    def bfs(self, start, end):
        queue = PriorityQueue()
        queue.put((0, start))
        seen = set([start])
        parents = dict()

        while not queue.empty():
            cur_node = queue.get(0)[1]
            if cur_node is not end:
                for neighbour in cur_node.neighbours:
                    if neighbour not in seen:
                        dist_to_neighbour = cur_node.neighbours[neighbour]
                        queue.put((dist_to_neighbour, neighbour))
                        parents[neighbour] = cur_node
                        seen.add(neighbour)
            else:
                path = self.get_path(start, end, parents)
                return path
        return None

    def get_path(self, start, end, parents):
        path = deque([end])
        cur_node = end
        while cur_node is not start:
            cur_node = parents[cur_node]
            path.appendleft(cur_node)
        return list(path)

class Node:
    def __init__(self, id):
        self.id = id
        self.neighbours = dict()

    def add_neighbour(self, to, weight):
        self.neighbours[to] = weight

I have a few ideas about what might be improved but I'm not sure how to do it (or even if it's worth doing).

There might be a better way to do get_node() instead of squashing it into 2 lines like how I've done it.
Maybe cur_node = queue.get(0)[1] should be 2 lines?
Change dist_to_neighbour = cur_node.neighbours[neighbour] to use a method instead of using dictionary notation (which might be a little awkward). Maybe I should have a method instead?
Using something else instead of a deque in get_path() (especially in an interview context where deque might be a bit too exotic).
Have more comments?

Here is my previous attempt at this code. I've also used the answers to my depth-first search attempts here and here to write this code.

I don't see that you are using add_node or get_node anywhere, but I'm assuming you would when actually using this class.

Nevertheless, I think these functions would better be called __setitem__ and __getitem__, allowing usage, such as g = Graph(); g[5] = Node(5) and print g[5]. This would also make initializing the graph easier, which leads me to my next recommendation.

I would expect the Graph class to take an argument on initialization to directly build a graph (without having to manually add every single node).

Lastly, get_node can be greatly simplified by using dict.get, which returns None when the key is not present in the dictionary.

class Graph:
    def __init__(self, nodes=None):
        self.nodes = dict()
        if nodes is not None:
            for _id in nodes:
                self[_id] = Node(_id)

    def __setitem__(self, _id, val):
        self.nodes[_id] = val

    def __getitem__(self, _id):
        return self.nodes.get(_id)

    def add_node(self, _id):
        self[_id] = Node(_id)

    ...

This allows initializing an instance like this: Graph([1,2,3]). It does not, however, allow initializing a nested structure.

Regarding your questions:

Use dict.get
I would maybe use _, cur_node = queue.get(0) to make it clearer that you don't need the priority here.
I think it is still quite readable.
deque does not seem too exotic to me, but then again, I don't perform job interviews, so who know.

Last but not least, I would have a look at whether you really need queue.PriorityQueue here. One of its features is that it is thread-safe, because it performs locking when adding or popping values from the queue. However, it also incurs a performance hit because of this (I saw a factor 2 being discussed for Python 2.6).

Instead you could roll your own implementation using heapq, similar to the one shown in the implementation details for heapq, maybe without the part about REMOVED entries, because you don't change the priority of a neighbour once you have calculated it. So you could use:

from heapq import heappush, heappop
import itertools

class PriorityQueue:
    def __init__(self, tasks=None):
        '''A new PriorityQueue.
        Parameters:
            tasks:    iterable of (priority, task) tuples'''
        self.pq = []                         # list of entries arranged in a heap
        self.counter = itertools.count()     # unique sequence count

        if tasks is not None:
            for priority, task in tasks:
                self.add_task(task, priority)

    def add_task(self, task, priority=0):
        'Add a new task'
        count = next(self.counter)
        entry = (priority, count, task)
        heappush(self.pq, entry)

    def pop_task(self):
        'Remove and return the lowest priority task. Raise KeyError if empty.'
        if self.pq:
            _, _, task = heappop(self.pq)
            return task
        raise KeyError('pop from an empty priority queue')

This should basically be how queue.PriorityQueue is implemented as well, but without the locking. Note that heapq is implemented in C, so it is quite fast. You would have to make some performance comparisons to see how this performs with respect to queue.PriorityQueue.


Determining which vehicle Rob wants to buy Rob is looking to buy a used pickup truck. He will spend a maximum of
  $10,000 but would like to spend as little as possible. He will spend
  up to $3000 for a 2000 or older model, $5000 for a 2001-05 model,
  $7000 for a 2006-09 model, $9000 for a 2010 or newer model. Rob will
  not buy a red truck but he will pay 20% more for a white truck.

  Given the information above and using C# or VB.net, write a function
  using nested if/else statements that receives the following variables:

d_Price decimal

s_Color string

i_Year integer

  ...and returns a Boolean value to determine whether a truck is one Rob
  wants to buy.

I don't think is a good approach. How can this be improved?

 //Function to decide if the car is what Rob wants
static bool Rob_Wants(decimal d_Price, string s_Color, int i_Year)
{
    decimal [] max_array = {3000, 5000, 7000, 9000};  //max prices

    //if/else to check the condition
    if (s_Color == "Red" || d_Price>10000)
 return false;
    else if (s_Color == "White")
    {
        for (int i = 0; i < max_array.Length; i++)
            max_array[i] = max_array[i]+((max_array[i] * 20) / 100);
    }
    if (d_Price <= max_array[0] && i_Year >= 2000)
        return true;
    else if (d_Price <= max_array[1] && i_Year >= 2001)
        return true;
    else if (d_Price <= max_array[2] && i_Year >= 2006)
        return true;
    else if (d_Price <= max_array[3] && i_Year >= 2010)
        return true;
    else return false;
}

decimal [] max_array = {3000, 5000, 7000, 9000};  //max prices

Let's try something:

int[] maxPrices = {3000, 5000, 7000, 9000};

Or even:

var maxPrices = new[] {3000, 5000, 7000, 9000};

The cutoff prices in the problem statement are all integers, so you don't need to bother with a decimal and floating-point errors here. And since you're using an array initializer to initialize a local variable with a handful of integers, the type of maxPrices is obviously int[].

Notice also, that you stop needing the comments when the code can speak for itself: the informative and useful part of max_array is max - the rest of the useful meaning is hidden in a comment. Seeing // max prices next to a variable declaration tells me one thing: that this comment wouldn't need to be there if the variable was called maxPrices (notice camelCase).

Convention for locals and parameters, is camelCase. Your code suffers from a severe Notationis Hungarianum - symptoms being that d_ for "decimal", that s_ for "string" and that i_ for "integer". Don't do that. Here's why:

  vUsing adjHungarian nnotation vmakes nreading ncode adjdifficult.

  write a function using nested if/else statements

Your function is structured like this:

if
else if

for

if
else if
else if
else if
else

I see no nested if conditions. And you know what? That's awesome: nested ifs are usually not pretty, you'd normally want to avoid resorting to them.

Nested conditions would look like this:

if

if

if
else

else if

if
else

else

else

If doing that exercise should teach you one thing, it's to recognize that shape in your code. That shape has a name: it's arrow code, and it's usually not the code you want to be maintaining.

So, what then?

He will spend:

  up to $3000 for a 2000 or older model
  up to $5000 for a 2001-05 model
  up to $7000 for a 2006-09 model
  up to $9000 for a 2010 or newer model

Also, he:

  will not buy a red truck
  will pay 20% more for a white truck
  will pay no more than $10,000

I think I'd use the built-in Color enum, just because it's there and appropriate for this.

I'd first eliminate the red truck, and return early:

if (color == Color.Red || price > 10000) { return false; }

And your code does exactly that. Well done!

Now the fun part. You went with arrays, and a if..else if construct. The problem is that when you read the code you can't help but notice the [0], the [1], the [2] and the [3] one under the other, all in order.

If I could map a year to the maximum price I'm willing to pay for, my life would be much simpler!

var maxPrices = new Dictionary<int,int>
{
    { 2000, 3000 },
    { 2005, 5000 },
    { 2009, 7000 },
    { 9999, 9000 },
};

Then we can iterate the dictionary entries, return true and break out when the parameters compare favorably against the Key (year) and the Value (maximum price)

var result = false;
foreach(yearPrice in maxPrices.OrderBy(item => item.Key)) // note: item.Key is the year
{
    var maxPrice = yearPrice.Value;
    if (color == Color.White)
    {
        maxPrice *= 1.2;
    }

    if (year <= yearPrice.Key && price <= maxPrice)
    {
        result = true;
        break;
    }
}
return result;

I don't understand the requirement for nested if statements. Specs usually tell you what to do, not how to do it. And imposing nested constructs (or anything really) sounds like some kind of "coding game" - however if you want to be writing professional code, that isn't the kind of code you want to be writing. Or rather, maintaining.