Sprite is Refreshing I am using "AndEngine" and I would like to know if I am following the best practice when I want to refresh the entity.

Let say, I have 2 sprites and I want change them into 2 another sprites.

I usually do it like this :

class MainClass {
    private Sprite sprite1;
    private Sprite sprite2;

    private void someMethod(){
        sprite1 = new Sprite(0,0,someRegion1,getVertexBufferObjectManager());
        sprite2 = new Sprite(10,10,someRegion2,getVertexBufferObjectManager());

        attachChild(sprite1);
        attachChild(sprite2);
    }

    private void refreshSprites(Sprite pSprite1, Sprite pSprite2){
        Sprite temp1 = sprite1;
        Sprite temp2 = sprite2;
        sprite1 = sprite2 = null;
        runOnUpdateThread(new Runnable() {
                            @Override
                            public void run() {
                                temp1.detachSelf();
                                temp2.detachSelf();
                            }
                        });
        sprite1 = pSprite1;
        sprite2 = pSprite2;

        attachChild(sprite1);
        attachChild(sprite2);
    }
}

I am not familiar with AndEngine, but here are some things that I saw:

Inside your refreshSprites method you call detachSelf and attachChild in unsynchronized way. So either make sure this method is called from updateThread or make all those operation in your Runnable class. Here is an example of the second choice.

private void refreshSprites(Sprite pSprite1, Sprite pSprite2){
        final Sprite oldSprite1 = sprite1;
        final Sprite oldSprite2 = sprite2;
        final Sprite newSprite1 = pSprite1;
        final Sprite newSprite2 = pSprite2;

        sprite1 = sprite2 = null;

        runOnUpdateThread(new Runnable() {
                            @Override
                            public void run() {
                                oldSprite1.detachSelf();
                                oldSprite2.detachSelf();
                                attachChild(newSprite1);
                                attachChild(newSprite2);
                            }
                        });

    }

Pay attention to the final that I am using, otherwise those values will not be available in the oldSprite1 inner class.

And some small things

sprite1 = sprite2 = null; you should place in it's own method, call it reset or some thing similar. It will improve your method Single responsibility principle
Pay attention to my choice of variables names: oldSprite, I think it explain the meaning of the variable better than tmp.


Removing duplicates from multiple arrays at once This (working) code is supposed to return a new array that consists of elements of both arrays that were not marked as duplicates (find duplicates in both arrays, remove such elements from both arrays and create an array from the leftovers, if any)

Is there a way to simplify this function?

function diff(arr1, arr2) {
    var arr1_filtered, arr2_filtered;

    arr1_filtered = arr1.filter(function (v) {
        return arr2.indexOf(v) == -1;
    });

    arr2_filtered = arr2.filter(function (v) {
        return arr1.indexOf(v) == -1;
    });

    return arr1_filtered.concat(arr2_filtered);
}

Create a function on Array.prototype that'll accept an array and filter the array by checking if the element is in the other array.

// Define a function on Array prototype
Array.prototype.arrayDiff = function (arr) {
    return this.filter(function (v) {
        return arr.indexOf(v) === -1;
    });
};

function diff(arr1, arr2) {
    // 1. Remove elements that are in arr2 from arr1
    //    i.e. arr1.arrayDiff(arr2)
    // 2. Remove elements that are in arr1 from arr2
    //    i.e. arr2.arrayDiff(arr1)
    // 3. Concat the result of both the arrays

    return arr1.arrayDiff(arr2).concat(arr2.arrayDiff(arr1));
}

// Define a function on Array prototype
Array.prototype.removeDuplicates = function(arr) {
  return this.filter(function(v) {
    return arr.indexOf(v) === -1;
  });
};

function diff(arr1, arr2) {
  return arr1.removeDuplicates(arr2).concat(arr2.removeDuplicates(arr1));
}

var arr = diff([1, 3, 4, 2, 5], [2, 45, 7, 42, 1, 6]);
document.write(JSON.stringify(arr));


Compute histrogram bins for a large data.frame fast I know about apply, but I'm not sure how to write it so that it works for embedded for loops with if statements and so forth. It gets messy. I am a novice so if you can show me how to properly rewrite the following efficiently it should help a lot.

This is an example of what the dataframe inFile() should look like. In the code below I subset on the dataframe so that rows are between two dates, abs_start and abs_end (you'll notice the CSV has a dates column). I then am trying bin the rows into bins that are input$binning seconds long. I simply iterate through all the dates and for each date I iterate through all the bins until I find the one that a particular row belongs to. Then I add this bin's number to a list. After doing this for all rows in the dataframe I add the list as a new column to the data.frame as it now specifies which bin each row belongs to.

This is slow though. The user specifies the bin length and then I want the histrogram to automatically update (using R Shiny framework). How can I speed it up?

  # Subset between start and end with bin column and selected mice
  subsetTable<-reactive({
    subsetMice<-inFile()[inFile()[,TAG_COL]%in%input$tagChooser,]

    abs_start <- strptime(input$abs_start,"%Y-%m-%d %H:%M:%S",tz='US/Pacific')
    abs_end <- strptime(input$abs_end,"%Y-%m-%d %H:%M:%S",tz='US/Pacific')

    if(is.null(subsetMice)||nrow(subsetMice)<=0){
      return()
    }

    # subset between abs_start and abs_end
    convertedDates<-strptime(as.character(subsetMice[,DATE_COL]),"%Y-%m-%d %H:%M:%S",tz='US/Pacific')
    inF<-subsetMice[convertedDates>abs_start&convertedDates<abs_end,]
    convertedDates<-convertedDates[convertedDates>abs_start&convertedDates<abs_end]

    bins<-seq(abs_start,abs_end,by=as.integer(input$binning))
    bin_no<-length(bins)

    bin_nos<-c(NULL)
    bin_starts<-c(NULL)
    for(i in 1:length(convertedDates)){
     for(j in 1:length(bins)){
        # Check if at right bin
        if(!is.na(bins[j+1])){
          if(bins[j]<=convertedDates[i]&&convertedDates[i]<bins[j+1]){
            bin_nos[i] <- j
            bin_starts[i] <- bins[j]
            break
          }
        }
       else{
         stopifnot(bins[j]<=convertedDates[i])
         bin_nos[i] <- j
         bin_starts[i] <- bins[j]
       }
      }
    }
    cbind(inF,bin_starts,bin_nos)
    })

Within the algorithm you post you can take advantage of a few features:

if(bins[j]<=convertedDates[i]&&convertedDates[i]<bins[j+1]) - as you check the bins in order there is no need to check that the previous bin.

As explanation say if j is 5 then you check bins[5]<=convertedDates[i]&&convertedDates[i]<bins[6]
But in the next step j is 6 and you check. bins[6]<=convertedDates[i]&&convertedDates[i]<bins[7]

So you twice compare with bins[6] - if you think about it you only reach j=6 if bins[6]<=convertedDates[i]. So no need to check that again.

if you would sort the convertedDates before the comparison you can use that you can start the search of bins by the last found bin - instead of the first.
actually there is no need to loop over all bins - instead if you calculate (convertedDates - absStart) %/% input$binning it should tell you exactly which bin the date belongs.
it may also help slightly if you preallocate the output lists as shown here.

It might also help to read the source code of the hist function and see what "tricks" they use there.


Vector backed by memory pages The purpose of this class is to wrap a std::vector in a class so that never a new object is added. We don't allocate a new object on the stack but we trying to fit it on the current memory page, or create a new page for it.

#include <vector>

template<class T>
class  PagedVector {

public :
    PagedVector(int initialCapacity, float load_factor = 1.5f):
    pageSize(static_cast<size_t>(load_factor*initialCapacity*sizeof(T))),
    current_byte_offset(0),
    current_page(0)
    {
        data.reserve(initialCapacity);

        //initialise one page:
        char* ptr = static_cast<char*>(malloc(pageSize));
        pages.push_back(std::make_pair(ptr, pageSize));
    }

    template<class U>
    U* create() {
        char* memoryPtr = getNextAdress(sizeof(U), std::alignment_of<U>::value);
        U* objectPtr = new (memoryPtr) U();
        data.push_back(objectPtr);
        return objectPtr;
    }

    ~PagedVector() {
        //delete objects
        for(T* obj : data) {
            obj->~T();
        }

        //free memory
        for(auto& pair : pages) {
            free(pair.first);
        }
    }

    void reset() {
        //delete objects
        for(T* obj : data) {
            obj->~T();
        }
        data.clear();

        //keep memory but reset page count
        current_page = 0;
        current_byte_offset = 0;
    }

    const std::vector<T*>& getData() const {
        return data;
    }

private :
    std::vector<T*> data;
    std::vector< std::pair<char* , size_t > > pages;

    size_t pageSize;
    size_t current_byte_offset;
    int current_page;

    char* getNextAdress(size_t size, size_t align) {
        //get next available adress for type with given size and aligment

        char* current_page_adress = pages[current_page].first + current_byte_offset;
        char* end_page_adress = pages[current_page].first + pages.back().second;

        size_t remainder_align = reinterpret_cast<uintptr_t>(current_page_adress) % align;
        size_t extra_alignment_padding = remainder_align==0 ? 0 : (align - remainder_align);

        char* next_aligned_adress = current_page_adress + extra_alignment_padding;
        char* end_aligned_adress = next_aligned_adress + size;

        if(end_aligned_adress < end_page_adress) {
            current_byte_offset += size + extra_alignment_padding;
            return next_aligned_adress;
        }

        //no more space on the current page, have to reallocate a new one:

        //in the rare case where the object size > page size
        size_t nextPageSize = std::max(pageSize, size);

        char* ptr = static_cast<char*>(malloc(nextPageSize));
        pages.push_back(std::make_pair(ptr, nextPageSize));
        current_byte_offset = size;
        ++current_page;
        return ptr;
    }

    //non-copiable:
    PagedVector(PagedVector&);
    PagedVector& operator=(PagedVector&);
};

Usage:

PagedVector<Base> pv(5);
    pv.create<Base>();
    Derived* p = pv.create<Derived>();

I'm wondering if this is correct (not relying on UB). I've never dealt that much with placement new and manual memory management.

I usually focus on the design and this review won't be different. You seem to have created some custom universal allocator, that will place all the (different) objects in continuous space (a page) or create new page if needed. The whole space can be released all at once, but no individual objects. So...

void reset() {
    //delete objects
    for(T* obj : data) {
        obj->~T(); // <<< will not call the appropriate destructor if not virtual

This can be very problematic, unless T has virtual destructor, you should place some static_assert:

template<class T>
class  PagedVector {
    static_assert(std::has_virtual_destructor<T>::value,
        "Objects without virtual destructor cannot be used");

Similar problem is in create allowing any class, should allow derived only:

template<class U>
U* create() {
    static_assert(std::is_same<U,T>::value || std::is_base_of<T,U>::value,
        "Objects not derived from T are not allowed");

Minor note about = delete:

//non-copiable:
PagedVector(PagedVector&);
PagedVector& operator=(PagedVector&);

The above should be written with const and = delete to avoid using it inside the class:

//non-copiable:
PagedVector(const PagedVector&) = delete;
PagedVector& operator=(const PagedVector&) = delete;


Converting from `std::array>` to `void*` I'm doing some work with Vulkan, in this I need to pass a set of values of various types.
Currently I'm using std::array<std::variant<>> in my interface, but I must pass Vulkan a void*.
To convert to the void* I use this function:
template <uint32_t Size>
void* voidArray(std::array<std::variant<uint32_t,float>,Size>& array, uint32_t size) {
    char* bytes = new char[size];
    uint32_t byteCounter = 0;
    for(uint32_t i=0;i<array.size();++i) {
        if(float* flt = std::get_if<float>(&array[i])) {
            *reinterpret_cast<float*>(bytes+byteCounter) = *flt;
            byteCounter += sizeof(float);
        }
        else if(uint32_t* uin = std::get_if<uint32_t>(&array[i])) {
            *reinterpret_cast<uint32_t*>(bytes+byteCounter) = *uin;
            byteCounter += sizeof(uint32_t);
        }
    }
    void* data = static_cast<void*>(bytes);
    return data;
}

I get the size variable earlier within a larger function (names are a little more context specific):
// std::array<std::variant<uint32_t,float>,NumPushConstants>& pushConstants
std::optional<uint32_t> pushConstantSize = std::nullopt; // later used for `size`
auto size_fn = [](auto variant) -> uint32_t {
    using T = std::decay_t<decltype(variant)>;
    if constexpr (std::is_same_v<uint32_t, T>) { return sizeof(uint32_t); }
    if constexpr (std::is_same_v<float, T>) { return sizeof(float);}
};
pushConstantSize = std::accumulate(pushConstants.begin(),pushConstants.end(),0,
    [size_fn](uint32_t acc, auto variant) { return std::move(acc) + std::visit(size_fn,variant); }
);

Both of these implementations, most prominently the first rouse my suspicion of being bad.
Are they bad? Is there a better way? Am I missing something?

It's hard to provide overall feedback, since you don't want to provide any details. There's probably a much simpler way to structure all of this.
But some things about the specific code:

don't do manual memory management:
char* bytes = new char[size];

I hope this gets delete[]d somewhere. We should use a std::vector instead, and get a pointer from vector.data() when we actually need to.

use std::byte in C++17:
char* bytes = new char[size];

If we have it, we should use std::byte instead of char (since we're dealing with object representations in memory).

use memcpy, not reinterpret_cast; simplify:
    if(float* flt = std::get_if<float>(&array[i])) {
        *reinterpret_cast<float*>(bytes+byteCounter) = *flt;
        byteCounter += sizeof(float);
    }
    else if(uint32_t* uin = std::get_if<uint32_t>(&array[i])) {
        *reinterpret_cast<uint32_t*>(bytes+byteCounter) = *uin;
        byteCounter += sizeof(uint32_t);
    }

Due to "strict aliasing rules" I don't think this is technically correct (even if it apparently works). Our char array doesn't have floats in it, so we shouldn't pretend that it does and copy in a float. We should instead use std::memcpy to copy the underlying representation of the float into the array.
Note also that we're doing the exact same thing for both types, so we could probably do something like:
std::apply([&] (auto const& var) {
    using T = std::decay_t<decltype(var)>;
    std::memcpy(bytes + byteCounter, static_cast<void const*>(&var), sizeof(T));
    byteCounter += sizeof(T);
}, array[i]);

same again (again):
The same thing is true of the second code snippet:
std::optional<uint32_t> pushConstantSize = std::nullopt; // later used for `size`
auto size_fn = [](auto variant) -> uint32_t {
    using T = std::decay_t<decltype(variant)>;
    if constexpr (std::is_same_v<uint32_t, T>) { return sizeof(uint32_t); }
    if constexpr (std::is_same_v<float, T>) { return sizeof(float);}
};
pushConstantSize = std::accumulate(pushConstants.begin(),pushConstants.end(),0,
    [size_fn](uint32_t acc, auto variant) { return std::move(acc) + std::visit(size_fn,variant); }
);

We don't need the if statements:
auto size_fn = [] (auto const& var) {
    using T = std::decay_t<decltype(var)>;
    return sizeof(T);
};
std::size_t size = std::accumulate(pushConstants.begin(), pushConstants.end(), std::size_t{ 0 },
    [=](std::size_t acc, auto var) { return acc + std::visit(size_fn, var); }
);

std::move won't help with a trivially copyable type, and we should be using std::size_t for the size.
The optional also seems redundant; we always have a size, even if the array is empty.
This snippet should be made a whole complete function by itself.


Excel Retrieving Data from website through Internet Explorer I took a look at Speed up processing between VBA and IE, but I didn't see anything that addressed the speeding up of the retrieval of information via IE.

Since MS got rid of the stock retrieval database (MSN Money), there wasn't an easy way to get current stock prices. From what I gather, there are some add-ins available through the app store, but like usual, I'm stuck on 2007 doing this.

This was my attempt at a UDF for retrieving the stock price from http://www.msn.com/en-us/money/ which requires finding elements on the page and getting the value from within. It's.. not very elegant, but it works. What can I do to speed up the IE processes? I know, I know - screenupdating, etc

Option Explicit

Public Function GetTickerPrice(ByVal ticker As String) As Double

    Dim IE As Object
    Dim pageData As Object

    Set IE = CreateObject("InternetExplorer.Application")
    Dim URL As String
    URL = "http://www.msn.com/en-us/money/stockdetails/fi-126.1." & ticker & ".NAS?symbol=" & ticker & "=PRFIMQ"

    IE.Navigate URL

     Do Until IE.ReadyState >= 4
            DoEvents
     Loop

     Application.Wait Now + TimeSerial(0, 0, 5)

     Set pageData = IE.document

     GetTickerPrice = pageData.getElementsByClassName("precurrentvalue")(0).innertext

     Set IE = Nothing
     Set pageData = Nothing
End Function

Worksheet functions were not designed for this. User-defined worksheet functions were not made a feature of Excel to go and fetch data online - in my humble opinion, this is a terrible abuse of UDF's.

A function like this should be part of the definition of a class module that defines an object responsible for this task, and wrapped with an interface, say ITickerPriceProvider:

Public Function GetTickerPrice(ByVal ticker As String) As Double
End Function

Then your code moves into a class module with Implements ITickerPriceProvider, and the signature becomes this:

Option Explicit
Implements ITickerPriceProvider

Private Function ITickerPriceProvider_GetTickerPrice(ByVal ticker As String) As Double
    'your code
End Function

What gives? Now you can get fancy and write a macro that looks like this:

Public Sub UpdateTickerPrices()

    Dim provider As ITickerPriceProvider
    Set provider = New WebTickerPriceProvider

    Dim updater As TickerSheetUpdater
    Set updater = New TickerSheetUpdater

    updater.UpdateTickerPrices provider, TickerPricesSheet

End Sub

And the TickerSheetUpdater.UpdateTickerPrices method is where you go and implement the code that locates ticker strings (your worksheet must have a column with those, right?), fetches the prices for each one, and updates the worksheet.

This is a job for a macro, not a UDF.

As a bonus, the TickerSheetUpdater class can be unit-tested because the dependencies are under control - and now if somebody cuts the network cable, you can handle errors gracefully, instead of having 200 UDF calls blowing up one after another.

As for the implementation itself, I don't see any glaring issues. I would have named IE something like browser, URL would have been url, and there would have been an On Error GoTo CleanFail statement at the top, and a CleanExit label to ensure resources get cleaned up whether or not an error occurs:

Private Function ITickerPriceProvider_GetTickerPrice(ByVal ticker As String) As Double
    On Error GoTo CleanFail

    'implementation here

CleanExit:
    Set browser = Nothing
    Set pageData = Nothing
    Exit Function

CleanFail:
    'handle errors
    Resume CleanExit

End Function

Turning ScreenUpdating off isn't going to help much, at least not with a UDF approach - the function isn't updating any cell values, it is a cell value.

However I'd try to see if there wouldn't be a way to remove this:

Application.Wait Now + TimeSerial(0, 0, 5)

...or add a comment that explains why it's needed, and why it needs to be a whole 5 seconds - especially given you've just waited for a specific ReadyState on your IE object:

 Do Until IE.ReadyState >= 4
        DoEvents
 Loop

What's 4 anyway? Make an Enum for the possible values, and use it instead of hard-coding a value like this. Why do you need to wait 5 seconds after the browser gets into that ReadyState? Could you keep loop-waiting for a later state value? An enum would help understanding what's going on and why here.


Int extension for translating integer to plain English This code was inspired by this question: Number to Words

import Foundation

extension Int {
    func plainEnglish (negativeSign: String = "negative") -> String {
        func singleNumberName (number: Int) -> String {
            switch (number) {
            case 1: return "one"
            case 2: return "two"
            case 3: return "three"
            case 4: return "four"
            case 5: return "five"
            case 6: return "six"
            case 7: return "seven"
            case 8: return "eight"
            case 9: return "nine"
            default:return ""
            }
        }

        func tensNumberName (number: Int) -> String {
            switch (number) {
            case 2: return "twenty"
            case 3: return "thirty"
            case 4: return "fourty"
            case 5: return "fifty"
            case 6: return "sixty"
            case 7: return "seventy"
            case 8: return "eighty"
            case 9: return "ninety"
            default:return ""
            }
        }

        func teensNumberName (number: Int) -> String {
            switch (number) {
            case 0: return "ten"
            case 1: return "eleven"
            case 2: return "twelve"
            case 3: return "thirteen"
            case 5: return "fifteen"
            default:return singleNumberName(number) + "teen"
            }
        }

        var number = self
        var digits: [Int] = []

        while number != 0 {
            digits.append(abs(number%10))
            number /= 10
        }

        var plainEnglish: [String] = []

        var isTeen = false
        let totalPlaces = digits.count
        var currentPlace = 0

        for (index, digit) in enumerate(digits.reverse()) {
            currentPlace = totalPlaces - index
            if currentPlace % 3 == 0 && digit > 0 {
                plainEnglish.append(singleNumberName(digit) + " hundred")
            } else if (currentPlace + 1) % 3 == 0 {
                if digit == 1 {
                    isTeen = true
                    continue;
                } else {
                    isTeen = false
                    plainEnglish.append(tensNumberName(digit))
                }
            } else {
                if isTeen {
                    plainEnglish.append(teensNumberName(digit))
                } else {
                    plainEnglish.append(singleNumberName(digit))
                }
            }

            if currentPlace == 4 {
                plainEnglish.append("thousand")
            } else if currentPlace == 7 {
                plainEnglish.append("million")
            } else if currentPlace == 10 {
                plainEnglish.append("billion")
            }
        }

        let initial = (self < 0) ? negativeSign : ""

        func combine (first: String, second: String) -> String {
            return first + " " + second
        }

        let finalString = plainEnglish.reduce(initial, combine: combine)

        return finalString.stringByTrimmingCharactersInSet(NSCharacterSet.whitespaceCharacterSet())
    }
}

This is implemented as an extension on the Int data type, so you can use it via dot notation on any integer (including literals).  Example usage:

let payamount = 98327
let dollars = payamount / 100
let cents = payamount % 100
let checkString = dollars.plainEnglish() + " dollars and " + cents.plainEnglish() + " cents"

Which results in a string that reads:

"nine hundred eighty three dollars and twenty seven cents"

So Apple already has an implementation for this, using NSNumberFormatter.  A bit easier to implement than what you have now ;).

var numberFormatter:NSNumberFormatter = NSNumberFormatter()
numberFormatter.numberStyle = NSNumberFormatterStyle.SpellOutStyle
var string = numberFormatter.stringFromNumber(100)


C# HTTP proxy performance I'm implementing HTTP traffic monitor (proxy) for my project right now. It's performance is satisfactory. I've updated regex settings and improved performance but I think there are also ways to make it better.

//mipt
public class TrafficMonitor
{
    private int _port;
    private bool _isWorking;
    private TcpListener _listener;
    private static readonly Regex myReg = new Regex(@"Host: (((?<hostName>.+?):(?<port>\d+?))|(?<hostName>.+?))\s+",
                RegexOptions.Compiled);
    public TrafficMonitor(int port)
    {
        _port = port;
        _listener = new TcpListener(IPAddress.Parse("127.0.0.1"), _port);
    }

    public void Start()
    {
        _listener.Start();
        _isWorking = true;

        while (_isWorking)
        {
            TcpClient client = _listener.AcceptTcpClient();
            Task.Factory.StartNew(() => Worker(client.Client));
        }
    }

    public void Stop()
    {
        _isWorking = false;
        _listener.Stop();
    }

    private void Worker(Socket clientSocket)
    {
        if (clientSocket.Connected)
        {
            byte[] httpRequest = ReadToEnd(clientSocket);

            Match m = myReg.Match(Encoding.ASCII.GetString(httpRequest));
            string hostName = m.Groups["hostName"].Value;
            int port = 0;

            if (!int.TryParse(m.Groups["port"].Value, out port))
            {
                port = 80;
            }
            IPHostEntry hostEntry = Dns.GetHostEntry(hostName);
            IPEndPoint endPoint = new IPEndPoint(hostEntry.AddressList[0], port);

            Socket socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
            socket.Connect(endPoint);
            if (socket.Send(httpRequest, httpRequest.Length, SocketFlags.None) == httpRequest.Length)
            {
                byte[] httpResponse = ReadToEnd(socket);
                if (httpResponse != null && httpResponse.Length > 0 )
                    clientSocket.Send(httpResponse, httpResponse.Length, SocketFlags.None);
            }
            socket.Close();
            clientSocket.Close();
        }
    }

    private byte[] ReadToEnd(Socket socket)
    {
        byte[] recievedData = new byte[socket.ReceiveBufferSize];
        int len = 0;

        using (MemoryStream m = new MemoryStream())
        {
            while (socket.Poll(1000000, SelectMode.SelectRead) && (len = socket.Receive(recievedData, socket.ReceiveBufferSize, SocketFlags.None)) > 0)
            {
                m.Write(recievedData, 0, len);
            }

            return m.ToArray();
        }
    }
}

I don't program in C and I don't know the exact input strings that you're expecting, but I can offer a couple of regex suggestions based on reasonable assumptions.

Your regex:

@"Host: (((?<hostName>.+?):(?<port>\d+?))|(?<hostName>.+?))\s+"

Your pattern looks for a hostname followed by a colon then a port number then whitespace characters OR a hostname followed by whitespace characters.

Since you are not capturing the whitespace and merely using the whitespace as an end point, the pattern can be reconfigured.
Since the colon-port substring is optional, the zero or one quantifier ? can be used to eliminate the redundant named capture group (which was causing trouble at regex101.com).
+? calls for "lazy" matching.  If you want your pattern to execute quickly, rewrite your pattern to use greedy quantifiers.

New pattern:

@"Host: (?<hostName>[^:\s]+)(:(?<port>\d+))?"

Here is a cheap and cheerful demo: https://regex101.com/r/NqSJ8L/1/

Note, my above pattern could be massaged a few different ways.  For instance, you might use ^ and an m pattern modifier to anchor the pattern to the start of the line of text.  Again, I don't program in C, so I don't know if there are incapatabilities with certain pattern entities.

Or instead of a negated character class, you can list the valid character for a hostname:

@"Host: (?<hostName>[\w.-]+)(:(?<port>\d+))?"

P.s. It just occurred to me that my optional pattern will not always deliver a named capture group for the port, so if you don't want to write code to check that the port capture group is generated, here is OhMyGoodness's commented pattern without the trailing whitespace matching.  (?<= ) is a lookbehind. (?: ) is a non-capturing group.

Host: (?<hostName>[\w.-]+):?(?<port>(?:(?<=:)\d+)?)


Function that divides money with regard to the smallest denomination I'm writing a function divideMoney that takes the value of a sum of money and divides it according to some percentages, then rounds each division to the nearest denomination.

For example:

//divideMoney(sum, percentages, denomination);

divideMoney(102, [0.5, 0.30, 0.20], 0.25);

This should divide the sum of $102 to 3 divisions, first is 50% ($51), second is 30% ($30.6) and the third is 20% ($20.4).

But because the smallest accepted denomination is the quarter ( 0.25 ), so $30.6 should be $30.50 instead, and $20.40 should be $20.50 as well. So the expected result is [51, 30.5, 20.5].

Here is what I tried

Original Code

function divideMoney(sum, percentages, denomination) {
    var result, diff, resultSum;
    result = [];
    diff = 0;
    percentages.forEach(function (percent) {
        var div, fraction, mod;
        div = (percent * sum) + diff;
        fraction = div - Math.trunc(div);
        if (fraction > 0) {
            mod = fraction % denomination;
            diff += mod;
            div -= mod;
        }
        result.push(div);
    });
    resultSum = result.reduce(function(a, b){ return a + b; }, 0);
    if (resultSum < sum) {
        result.push(sum - resultSum);
    }
    return result;
}

Update (fix rounding)

function divideMoney(sum, percentages, denomination) {
    var result = [], remainder = sum, part, i;
    for(i = 0; i < percentages.length - 1; i++) {
        part = percentages[i] * sum;
        if (part % 1) {
            part = Math.round(part/denomination)*denomination;
        }
        remainder -= part;
        result.push(part);
    }
    result.push(remainder);
    return result;
}

I can make infinite money (well, as much as will fit in a javascript number) with your code!

var money = 10;
while(true){
    money = divideMoney(money, [3, 0], 1)[0];//muhahahahah infinite money
}

Which divides the money like so: 1 for me, 1 for myself, 1 for I, none for you. 1 for me...

I'd recommend in this case to fall back on relative percentages: that is, sum the percentages and then set the cut to be the percentage of the summed percentage. So 10, 5, 5 = 20 total, 10/20 = 0.5, 5/20 = 0.25, so it's 0.50, 0.25, 0.25.


Extracting lines from a bytearray This class is part of a utility that reads lines from a set of nonblocking file descriptors, blocking only when there are no complete lines to emit.

class NonblockingLineBuffer:
    def __init__(self, fd, encoding):
        self.fd  = fd
        self.enc = encoding
        self.buf = bytearray()

    def absorb(self):
        while True:
            try:
                block = os.read(self.fd, 8192)
            except BlockingIOError:
                return

            if block:
                self.buf.extend(block)
            else:
                self.is_open = False
                # We don't close the file here because caller
                # needs to remove the fd from the poll set first.
                return

    def emit(self):
        def emit1(chunk):
            self.emitted_this_cycle = True
            return (self.fd, chunk.decode(self.enc).rstrip())

        buf = self.buf
        self.emitted_this_cycle = False
        while buf:
            r = buf.find(b'\r')
            n = buf.find(b'\n')
            if r == -1 and n == -1:
                if not self.is_open:
                    yield emit1(buf)
                    buf.clear()

            elif r == -1 or r > n:
                yield emit1(buf[:n])
                buf = buf[(n+1):]

            elif n == -1 or n > r:
                yield emit1(buf[:r])
                if n == r+1:
                    buf = buf[(r+2):]
                else:
                    buf = buf[(r+1):]

        self.buf = buf
        if not self.is_open:
            self.emitted_this_cycle = True
            yield (self.fd, None)

This question is specifically about emit, which is complicated, confusing, and might not be as efficient as it could be.  Please suggest ways to make it less complicated and/or confusing, and more efficient.

(I know it could be much simpler if it didn't need to reimplement universal newline handling, but that is unfortunately a requirement from the larger context.)

(If there's something in the standard library that does some or all of the larger task, that would also be a welcome answer.)

You have overlooked a corner case: while you normally treat \r\n as a single separator, this not the case when the two bytes are split between blocks.
splitlines could handle the universal newlines for you.

Here's what I came up with; still not very pretty I'm afraid. Initialize self.carry_cr = False in constructor.

def emit(self):
    buf = self.buf
    if buf:
        # skip \n if previous buffer ended with \r
        if self.carry_cr and buf.startswith(b'\n'):
            del buf[0]
        self.carry_cr = False

    lines = buf.splitlines()
    if buf:
        if self.is_open and not buf.endswith((b'\r', b'\n')):
            buf = lines.pop()
        else:
            if buf.endswith(b'\r'):
                self.carry_cr = True
            del buf[:]
    self.buf = buf

    self.emitted_this_cycle = False
    if lines:
        self.emitted_this_cycle = True
        for line in lines:
            yield (self.fd, line.decode(self.enc).rstrip())
    if not self.is_open:
        self.emitted_this_cycle = True
        yield (self.fd, None)


Recursive function and memorization to find minimum operations to transform n to 1 I'm a new, self-taught programmer working on the Google FooBar challenge.  I've submitted my answer (code at bottom) and it was accepted, but I'd like suggestions on how to improve my solution.

Challenge: return minimum number of operations to transform a positive integer to 1. Valid operations are: n+1, n-1, or n/2.

My solution
I started with a recursive function, but ran into a runtime error with very large numbers.  I added memorization using a global variable to store values that had already been computed, but this seems inelegant.  (I think it's discouraged to use global variables?)

Suggestions on how I might improve the below solution?

paths = {1:0, 2:1}

def shortest_path(num):
    if num in paths:
        return paths[num]

    if num % 2 == 0:
        paths[num] = 1 + shortest_path(num / 2)
    else:
        paths[num] = min(2 + shortest_path((num+1)/2),
                         2 + shortest_path((num-1)/2))
    return paths[num]

def answer(n):
    num = int(n)
    return shortest_path(num)

Test cases:

n = 15 --> 5
n = 293523 --> 25
n = 191948125412890124637565839228475657483920292872746575849397998765432345689031919481254128901246375658392284756574839202928727465758493979987654323456890319194812541289012463756583922847565748392029287274657584939799876543234568903 --> 1029

The input number can be up to 309 digits long hence the final test case

The problems you've encountered usually signal that the approach is not the best.

Consider the very first step in your algorithm: you are definite that if the number is even, the best action is \$n\rightarrow \frac{n}{2}\$ (why?).

Try to apply the same logic one step further. Let \$n\$ be odd. Either \$\frac{n+1}{2}\$ or \$\frac{n-1}{2}\$ is also odd (why?). The best action is to pick that which is even (why?).

As soon as you prove all the why statements, a straight-forward non-recursive algorithm is easy to obtain.


Scroll text across a fixed width In an interview, I was asked to write code to produce this output:

    T
   TE
  TES
 TEST
 EST
 ST
 T

The code I wrote was:

#include<iostream>
int main() {
char* a[] = {"t","e","s","t","\0"};
int k=0, i,j;
for(i=0; i<4; i++) {
    while(k<3) {
        cout<<" ";
        k++;
    }
    k=i+1;
    for(j=0; j<=i; j++) {
        cout<<a[j];
    }
    cout<<"\n";
}
for(i=1; i<4; i++) {
    for(j=i; j<4; j++) {
        cout<<a[j];
    }
    cout<<"\n";
}
return 0;
}

I think the code is not precise also the complexity is really a lot more that what I expected.

How can I improve this code in terms of time and space complexity?

I was wondering if additional sets of eyes could spot potential errors. Also, any stylistic or performance improvement suggestions would be appreciated

Syntax

First things first - I had to fix a handful of syntax errors for this to compile.  The most glaring is the misspelling of std::cout in many places.  The other bug is the forbidden conversion of string literals from const char* to char* - fixed by declaring a as const char* a[].

Style

The code is very dense - more whitespace around operators will help readability.  The one-letter variable names also hinder comprehension - i and j are obviously loop indexes, but what's k for?

These style issues are especially important in interviews - your interviewer wants to know whether you can write code that other employees are able to work with.

Structure

Everything here is crammed into main().  That's not great practice in general, as it makes your code hard to re-use.  Prefer to write a function - that's more amenable to unit test, allowing you to specify different strings to scroll, and to capture the output (e.g. into a std::ostringstream).

Algorithm

Rather than writing a single character at a time, it's probably easier to work with a std::string.  If we pad both ends with spaces, it's then just a matter of selecting the right substring to print at any given time.

Re-writing the code

I'd start by defining the interface I want to use.  Something like this:

#include <iostream>

int main()
{
    scroll_text(std::cout, "TEST", 4);
}

Now we know how we want to call it, let's write the signature:

void scroll_text(std::ostream& out,
                 const std::string& text,
                 unsigned width)

And now implement using the padding strategy I mentioned:

const std::string padding(width-1, ' ');
auto padded_text = padding + text + padding;

for (unsigned i = 0;  i <= padded_text.length() - width;  ++i) {
    out << padded_text.substr(i, width) << '\n';
}

Putting this all together:

#include <ostream>
#include <string>

void scroll_text(std::ostream& out,
                 const std::string& text,
                 unsigned width)
{
    const std::string padding(width-1, ' ');
    auto padded_text = padding + text + padding;

    for (unsigned i = 0;  i <= padded_text.length() - width;  ++i) {
        out << padded_text.substr(i, width) << '\n';
    }
}

// Test program

#include <iostream>

int main()
{
    scroll_text(std::cout, "TEST", 4);
}

Advanced

We could make the code more efficient by using std::string_view to reduce the amount of copying.  I won't show that here, but you might consider it as an exercise.

Another exercise - make the width argument optional, and if not specified, then use the length of the input string as the width.


Simplifying Radical Expressions I'm working on a program to handle simplifying square roots (radicals). My biggest issue is, in order to stay precise, I can't have input any longer than 15 digits. Whenever a number is longer than 15 digits, it loses precision when converted to double when I try to determine if the square is actually a factor. I realize I can just use an int and call it a day, but I'd really like to be able to use bigger numbers. Is there a better way I can do this to handle numbers longer than 15 digits? Also, any other optimization or readability ideas would be greatly appreciated! This is the best algorithm I could come up with for finding square roots.

import java.util.Scanner;

public class SimpRad
{
    public static final String SQUARE_ROOT_SYMBOL = "\u221A";
    public static final int INPUT_FAILURE = -1;

    static long[] simplify( long square )
    {
        double outside = 1;
        double inside = square;
        long[] simplified = { 1, square };

        // Check if it's already a perfect square
        outside = Math.sqrt(square);
        if (outside == Math.floor(outside))
        {
            simplified[0] = (long)outside;
            simplified[1] = 1;
            return simplified;
        }

        // Find all the squares that could be factors and see if they are
        for (long factor = 2, sqr = 4; sqr <= (square / 2); factor++, sqr = (factor * factor))
        {
            // Is this square a factor?
            double in = (double)square / sqr;
            if (in == Math.floor(in))
            {
                simplified[0] = factor;
                simplified[1] = (long)in;
            }
        }

        // Otherwise, since it hasn't simplified, return the original radical
        return simplified;
    }

    public static String display( long radicand, long multiplier, long newRad, boolean negative )
    {
        String i = negative ? "i" : "";
        String negativeStr = negative ? "-" : "";

        // If it's already simplified
        if (multiplier == 1)
        {
            return (SQUARE_ROOT_SYMBOL + negativeStr + radicand + " = " + i
                        + SQUARE_ROOT_SYMBOL + newRad);
        }
        // If it's a perfect square
        else if (newRad == 1)
        {
            return (SQUARE_ROOT_SYMBOL + negativeStr + radicand + " = " + multiplier + i);
        }
        else
        {
            return (SQUARE_ROOT_SYMBOL + negativeStr + radicand + " = " + multiplier + i
                        + SQUARE_ROOT_SYMBOL + newRad);
        }
    }

    public static void main( String[] args )
    {
        Scanner userInput = new Scanner(System.in);
        long radicand;
        long[] rads;
        String radStr;
        boolean isNegative = false;

        do
        {
            System.out.print("Please enter the current radicand:  ");
            radStr = userInput.next();

            try {
                radicand = Long.parseLong(radStr);
            } catch (NumberFormatException e) {
                System.out.print("That's not an integer! ");
                radicand = INPUT_FAILURE;
                continue;
            }

            if (radicand < 0)
            {
                radicand = -radicand;
                isNegative = true;
            }

            if (radicand > 999999999999999l)
            {
                System.out.print("Please enter a radicand that is 15 digits or less. ");
                radicand = INPUT_FAILURE;
            }
        } while (radicand < 0);

        rads = simplify(radicand);
        System.out.println(display(radicand, rads[0], rads[1], isNegative));
    }
}

Better approach

First, if you got rid of all floating point and just use long, that would fix your inaccuracy problems.

Second, your loop is trying to find the greatest factor of the number that is a square.  But for a large number it will run for a long time because you check every number up to \$\sqrt n\$.  You can shorten the time by dividing any factors you find along the way.  For example, if 2 divides into \$n\$ 7 times, then \$2^3\$ can be factored out of the square root  and \$2\$ will remain under the radical.  I.e.:

If: \$n = 2^7 * m\$
\$\sqrt n = 2^3 \sqrt {2 * m}\$

If a factor divides an even number of times into your number, then it can be pulled out without leaving anything under the radical.

After you simplify for each factor, you keep going to find factors of \$m\$, which is smaller, so you only need to check factors up to \$\sqrt m\$.  You just need to keep track of the internal and external factors as you go.

In the worst case (if the number is prime), it will still take the same amount of time.  You could research factorization algorithms if you want something better.


Retrieve Tasks by Employee I think I've done a decent job keeping this query simple and understandable, but I'd like to know if you have some more advice.

My relevant entities are defined like this:

public class Task
{
    [ForeignKey("Employee")]
    public string EmployeeId { get; set; }
    public Employee Employee { get; set; }

    [Required]
    [ForeignKey("Status")]
    public int StatusId { get; set; }
    public TaskStatus Status { get; set; }

    [...]
}

public class Employee
{
    [Key]
    [DatabaseGenerated(DatabaseGeneratedOption.None)]
    [Required]
    [MaxLength(10)]
    [Index(IsUnique=true)]
    public string EmployeeId { get; set; }

    [Required]
    [StringLength(100)]
    public string FirstName { get; set; }

    [Required]
    [StringLength(100)]
    [DisplayName("Last name")]
    public string LastName { get; set; }

    [...]
}

public class TaskStatus
{
    [Key]
    public int Id { get; set; }

    [Required]
    [MaxLength(50)]
    public string Name { get; set; }

    [...]
}

I've stripped off what I believe are irrelevant parts of the entities.

The query itself is following:

// Retrieves all tasks from the database. We filter the tasks on 3 different criteria. Since all 3 criteria
// must be fulfilled for the task to be displayed, we use logical and to concatenate them.
//
// The first criteria is the search term. If the search term is empty, we don't filter out any tasks, so
// that is the first check we make. Also, we test if the FistName, LastName or the EmployeeId contain the
// search term.
//
// Next criteria is the status filter. We keep the tasks that have the right status, or all tasks if the
// given status is null.
//
// The last criteria is employee filter, and the logic is the same as for the status filter.
var tasks = db.Tasks
    .Include(t => t.Employee)
    .Include(t => t.Status)
    .Where(t =>
        (
            search == null
            || t.Employee.FirstName.Contains(search)
            || t.Employee.LastName.Contains(search)
            || t.Employee.EmployeeId.Contains(search)
        )
        && ((statusFilter == null) || (t.StatusId == statusFilter.Value))
        && ((employeeFilter == null) || (t.EmployeeId == employeeFilter))
    );

The query does what it is supposed to do, it filters the task list properly on all 3 parameters. I'm just worried that it is a little too unreadable (too tricky), especially since my colleagues are mostly young and inexperienced.

Remove the additional ()s in the statusFilter and employeeFilter lines as they are unnecessary.
Indent the statusFilter and employeeFilter lines so that they are symmetrical with the search lines.
Change search to be searchTerm as it is more specific.  search by itself could mean anything like a stored query, a boolean value, etc.
As per Magus' comment you can change t to task since it is more clear and will not add space that starts pushing statements across extra lines.

var tasks = db.Tasks
  .Include(task => task.Employee)
  .Include(task => task.Status)
  .Where(task =>
    (
      searchTerm == null ||
      task.Employee.FirstName.Contains(searchTerm) ||
      task.Employee.LastName.Contains(searchTerm) ||
      task.Employee.EmployeeId.Contains(searchTerm)
    ) &&
    (
      statusFilter == null ||
      task.StatusId = statusFilter.Value
    ) &&
    (
      employeeFilter == null ||
      task.EmployeeId == employeeFilter
    )
);


Raising error or returning None I have a method poker defined as below:

def poker(hands):
'''Returns the best hand from a list of hands.

    Parameters:
        hands: list

    Example:
        poker(hands) => hands
'''
    return max(hands, key=hand_rank)

As mentioned in the docstrings, this function returns the best hand from a list of hands.
The issue I am facing is while passing an empty list of hands to this function. Hypothetically and for greater coverage, I want to add such a test case.

Should I raise an error or should I return None object ?

Generally, how do I decide where an error should be raised and where should it not?

There is no universal rule and this particular decision is use-case specific. Note that there are other options as well, like returning -1 for an empty list.

If we consider that max() naturally does not work with empty sequences:

In [1]: max([])
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-4-8d2766aecf92> in <module>()
----> 1 max([])

ValueError: max() arg is an empty sequence

Then, throwing an error makes more sense in this particular situation. If you do so, you can empower your docstring with a doctest:

def poker(hands):
    """
    Returns the best hand from a list of hands.

        Parameters:
            hands: list

        Example:
            poker(hands) => hands

    Raises a ValueError in case of empty list of hands:

    >>> poker([])
    Traceback (most recent call last):
      ...
    ValueError: Empty list of hands.
    """
    if not hands:
        raise ValueError("Empty list of hands.")

    return max(hands, key=hand_rank)


The simple solution is to add a j loop.

void array_print(const int *ptr) {
  for (int j = 0; j < 10; j++) {
    for (int i = j; i < SIZE; i=i+10) {
      printf("%10d", ptr[i]);
    }
    printf("\n");
  }
}

Also look to a wider than "%10d" to cope with "1234567890" running together as well as 11 character "-1234567890".  Perhaps " %11d"?

Better code would pass in int columns, int rows, int size than hard code them.


Using Python to tokenize a string in a stack-based programming language As an avid user of esoteric programming languages, I'm currently designing one, interpreted in Python.

The language will be stack-based and scripts will be a mix of symbols, letters, and digits, like so:

ab123c-43=f54g6!6

This will be read from the file and passed to a new Script object, to tokenize. It needs to be tokenized into a list of strings, where adjacent digits are grouped together and converted into ints, but letters are individual, resulting in this array:

['a', 'b', 123, 'c', '-', 43, '=', 'f', 54, 'g', 6, '!', 6]

Currently, I am doing this using regex, with the following code (get_tokens is the main method):

import re

class Script:
    def __init__(self, text):
        self._tokens = Script.get_tokens(text)

    def get_tokens(text):
        """ Static tokenizer function.
        Uses regex to turn the string into a list of tokens.
        Groups adjacent digits together and converts to integers. """

        tokens = re.findall('\d+|[^\d]', text)

        for i in range(len(tokens)):
            if tokens[i].isdigit():
                tokens[i] = int(tokens[i])

        return tokens

    def run(self):
        raise NotImplementedError()

The resulting tokens can be seen with Script("ab123c-43=f54g6!6")._tokens.

I have a few questions about my method:

Is there any more Pythonic or elegant way to do this? (e.g, an alternative instead of regexing)
Should I really be using a static function like that, or should I move it outside the class?
If the loop is to be kept, should I be using enumerate instead?

for i,t in enumerate(tokens):
    if t.isdigit():
        tokens[i] = int(t)

It would be good practice to use r'raw strings' for regexes, especially since this one contains a backslash.  Instead of [^\d], you could use \D, or better yet, just ., since any digits would have been slurped up by the \d+ already.

Testing .isdigit() feels redundant to me, since the regex was already detecting digits.  If you use capturing parentheses, then you could immediately tell which portion of the regex matched, and avoid having to make a second pass to convert the integers.

If, as you say, this Script is meant to be interpreted, consider making _tokens an iterator rather than a list, which saves you from having to parse the entire string at once.  To that end, I recommend re.finditer() rather than re.findall().

I don't see any need to define a public method for tokenizing.  The generator expression could be written directly within the constructor:

import re

class Script:
    def __init__(self, text):
        self._tokens = (
            match.group(1) or int(match.group(0))
            for match in re.finditer(r'\d+|(.)', text)
        )


Decode string with hex character codes to UTF-8 characters From a system we receive messages that contain codes that represent UTF-8 characters.

For example :

var str="Test =64 =C2=AE =E1=A6=92 test";

To decode these codes to utf-8 I've added a simple function that does 3 regex replacements:

protected static string ReplaceHexCodesInString(string input)
{

    var output = input;
    var encoding = Encoding.UTF8;

    var regTripleHex = new Regex("=(E[0-9A-F])=([0-9A-F]{2})=([0-9A-F]{2})");
    output = regTripleHex.Replace(output, m => encoding.GetString(new[]{
            byte.Parse(m.Groups[1].Value, System.Globalization.NumberStyles.HexNumber),
            byte.Parse(m.Groups[2].Value, System.Globalization.NumberStyles.HexNumber),
            byte.Parse(m.Groups[3].Value, System.Globalization.NumberStyles.HexNumber)
        }));

    var regDoubleHex = new Regex("=([C-D][0-9A-F])=([0-9A-F]{2})");

    output = regDoubleHex.Replace(output, m => encoding.GetString(new[]{
            byte.Parse(m.Groups[1].Value, System.Globalization.NumberStyles.HexNumber),
            byte.Parse(m.Groups[2].Value, System.Globalization.NumberStyles.HexNumber)
        }));

    var regRemainingHex = new Regex("=([0-9A-F]{2})");

    output = regRemainingHex.Replace(output, m => encoding.GetString(new[]{
            byte.Parse(m.Groups[1].Value, System.Globalization.NumberStyles.HexNumber)
        }));

    return output;
}

This seems to work as expected for what's currently in those messages.
Note that messages don't contain 4-bit utf-8 characters
(f.e. 0xf0 0x90 0x8c 0xb8 = 𐌸)

But can this be simplified?
Perhaps there's already a standard function?

I searched, but haven't found a good standard build-in C# function that already does this type of conversion.

Well, except for an example that uses a function from System.Net.Mail.
But it seems very error-prone and requires a very specific format.

var input = "bl=61=C2=B0";
var output = System.Net.Mail.Attachment.CreateAttachmentFromString("", "=?utf-8?Q?" + input.Trim() +"?=").Name;

Are you willing to use % instead of =?

If so Uri.UnescapeDataString shall be sufficient. if not you can always Replace("=", "%") and use UnescapeDataString anyway.

Uri.UnescapeDataString("Test =64 =C2=AE =E1=A6=92 test".Replace("=", "%"))
//Test d ® ᦒ test


Exceptions or something else? I'm in the process of creating a program that can model points, vectors, lines and segments. I have the classes built and confirmed to be working correctly. The classes are built to handle multiple dimensions, so as an example, the same point class is used for R1, R2, R3, ... Rn.

The issue that I am dealing with is how to handle operations between classes if the dimensions are different. For example, you can add two vectors that are in R2 but you cannot add a vector that is in R2 and another in R3.

My original plan was to create an exception class that I would throw if the dimensions between the two objects was different. This would work but would entail needing to wrap a lot of operations in try/catch blocks. Is this the best course of action to take or is there a better method that i could impliment?

Below is my point class:

public class Point
{
    private List<double> values;

    public List<double> VALUES
    {
        get { return values; }
        set { values = value; }
    }
    public int DIMENSION
    {
        get { return VALUES.Count; }
    }

    public Point(List<double> args)
    {
        values = new List<double>();
        this.VALUES = args;
    }
    public override string ToString()
    {
        string temp = "( ";

        for (int i = 0; i < DIMENSION; i++)
        {
            temp += VALUES[i];

            if(i != DIMENSION - 1)
                temp += ", ";
        }

        temp += ");";

        return temp;
    }

    public static double Distance(Point point1, Point point2)
    {
        if (!SameDimension(point1, point2))
        {
            //add exception
        }

        double temp = 0;

        for (int i = 0; i < point1.DIMENSION; i++)
        {
            temp += Math.Pow(point2.VALUES[i] - point1.VALUES[i], 2);
        }

        return Math.Sqrt(temp);
    }
    public static Point Midpoint(Point point1, Point point2)
    {
        if (!SameDimension(point1, point2))
        {
            //add exception
        }

        List<double> temp = new List<double>();

        for (int i = 0; i < point1.DIMENSION; i++)
        {
            temp.Add((point2.VALUES[i] + point1.VALUES[i]) / 2);
        }

        return new Point(temp);
    }
    //This is the method used to check dimensions
    public static bool SameDimension(Point point1, Point point2)
    {
        if (point1.DIMENSION != point2.DIMENSION)
            return false;
        else
            return true;
    }
}//working, needs exceptions added

This is the most simple class but all check the dimension by using this meathod.

EDIT:
The reason for needing to encase operations in try/catch blocks is say I have:

Point p1 = new Point(new List<double> { 0, 0});
Point p2 = new Point(new List<double> { 3, 3});
Point p3 = new Point(new List<double> { 4, 4, 4});

double distance = Point.Distance(p1, p2);

This would work completely fine, giving the distance of 3sqrt(3). But if this was tried:

distance = Point.Distance(p1,p3);

An error should occur because you are trying to compare a point in 2-Dimensions to a point in 3-Dimensions. Code will be written to reduce the risk of this happening, but I want a safe-guard in place in case this situation arises. If I go with the exception route, try/catch blocks will be needed. This is more apparent in the Vector class where when you add two vectors a new vector is returned. In that case, if a dimension error is encountered, the return value would be null, causing the possibility of other errors. Using exceptions, in my opinion, would mitigate potential errors as well as give me a capacity to track when they occur during run-time.

Okay, I'll throw in a review. The part more directly addressing your question is at the end

Naming

Usually in C# you will see PascalCase used for public members. For private members, it's a bit more varied, but _camelCase (note the prefixed underscore) is quite popular. What I've never seen before is ALLCAPS for public members. You can of course choose your own casing conventions for your project, but in general it's a positive to be consistent with the conventions of the language. It's helpful when working in a group or contributing to open-source, and it's also helpful to keep code you write consistent in appearance with code you call from other libraries (including the core .NET libraries)
Casing aside, not all of your variables are well-named:

args is not a good name for the variable you pass in to the Point constructor. You already have a name for these, values, so why not keep consistent?
Having said that, Values isn't actually a great name either. A point doesn't have "values", it has "coordinates", so that might be a better name.
temp inside your ToString method isn't ideal either. It's declared on the first line of the method, and returned at the end, that's about as not-temporary as a local variable can get! I generally use result here, though I know some people hate it.
Likewise, temp inside your Distance and Midpoint methods can be renamed along the same principle of giving it a meaningful name which describes what the variable actually is.

Cleaning up code

A few bits and pieces that can be expressed more concisely and readably

You don't need to declare a separate backing field for VALUES, C# has some nice syntactic sugar to do this for you:

public List<double> VALUES { get; set; }

This works exactly the same way as what you have, except that since the values field no longer exists, you will have to update the first line of your constructor to use VALUES instead.
The SameDimension method can be much more concisely expressed as

public static bool SameDimension(Point point1, Point point2)
{
    return point1.DIMENSION == point2.DIMENSION;
}

You can also get rid of the loop (and the hard-to-name variable along with it) in ToString()

public override string ToString()
{
    return "( " + String.Join(", ", VALUES) + " )";
}

Don't Repeat Yourself

Generally this adheres pretty well to the Don't Repeat Yourself (DRY) principle. One place where I see repeated logic is in the check for the exception you're describing in the question. This can be moved out to its own method:

private static void ValidateSameDimension(Point point1, Point point2)
{
    if(!SameDimension(point1, point2))
        throw new MismatchingDimensionsException(); // Or whatever you decide to call it
}

Then you can just call ValidateSameDimension at the top of methods which need that validation.

Using Exceptions

Your approach, using an exception in the case of trying to perform an action between points of different dimensions, is good. But what you've written about it is actually a little contradictory. So trying not to move too far outside the realm of a code review, here's some advice about how should use exceptions.

As a basic review of exceptions, when an exception is thrown from a method, that method immediately terminates and it is passed up to the next method in the call chain (the one that called it), and continues being passed up until it is caught.

So let's say I write a method which calls DoFoo(), and I know that DoFoo() can throw a particular exception. My options are more or less as follows:

Surround DoFoo() with try...catch because I know how to fix (or attempt to fix) the problem that the exception represents, or because I know I can safely ignore the exception. An example of this is if DoFoo() involves a remote webservice call, and I want to retry the first time I get an exception from it, in case it was a temporary connectivity problem
Surround DoFoo() with try...catch but re-throw it from within the catch. This is because I can't actually handle or ignore it, but there's something else I need to do before kicking it on up the call chain. Maybe I want to log it, or maybe there's some important clean-up I need to do inside a finally block.
Surround DoFoo() with try...catch not because I can properly handle it, but because it's vital that my program doesn't actually terminate. This could be the case, for example, on a server-side process with an exception caused by a request sent by a client. It can't do anything more useful than log and display an error message to the client, but it needs to catch the exception so that it doesn't crash and stop processing requests altogether.
Don't surround DoFoo() with try...catch. This is when I can't do anything more useful than pass it on up through the call chain, either to be handled higher up, or bubble up all the way to terminate the program with an error.

So, looking through those options, you'll probably see that in your case, generally the best one will be the last. This will actually have the opposite effect to what you were worried about. Not trying means that your Vector (for example) won't silently fail and return null, instead it will cease operating and the exception will keep bubbling on up. On the other hand, if you do surround it in a try block and leave the catch empty, that would silently gobble up the exception, and that's where you're at risk of getting weird, hard to debug behaviour.


Memory cache implementation with a static class inside a normal class I'm trying to implement a cache available in a data provider (repository).

The data provider is a normal class, instanciated at every request made to the application.

The reason I have a static class is that I want all requests to share the same _lock object.

The data being cached doesn't change very often and is quite small (a few deserialized JSON documents) but they are often requested by other logic in the application.

Is this good design? I have test ran it under stress and it seems to work fine.

I think the locking in the Cache class should do what it is supposed to be doing (making sure data is not changed by two concurent requests and data that is being read is always up to date).

I also lock when reading from the database in the case where a request comes in and for some reason is slower than a second one (the first request would then populate the cache with possibly outdated data).

Are there any obvious flaws you can spot? Is the use of MemoryCache overkill in this case? I don't really need managing expiration and other options MemoryCache offers, so I'm split about just using a Dictionary.

public class DataProvider
{
    static SemaphoreSlim dbLock = new SemaphoreSlim(1);

    //This class has a bunch of methods to access the DB directly, when needed.

    /// <summary>
    /// Get items from the MemoryCache
    /// </summary>
    /// <typeparam name="T"></typeparam>
    /// <returns></returns>
    public async Task<List<T>> GetCacheItems<T>() where T : IDocumentModel
    {
        if (Cache.Items<T>() == null)
        {
            await dbLock.WaitAsync();
            try
            {
                var items = await GetItemsFromDatabase<T>();
                Cache.SetItems(items);
            }
            finally
            {
                dbLock.Release();
            }
        }

        return Cache.Items<T>();

    }

    /// <summary>
    /// Update a cached item when DataProvider updates one
    /// </summary>
    /// <typeparam name="T"></typeparam>
    /// <param name="item"></param>
    /// <returns></returns>
    public async Task UpdateCacheItem<T>(string itemData) where T : IDocumentModel
    {
        var item = JsonConvert.DeserializeObject<T>(itemData);
        var cachedItems = await GetCacheItems<T>();

        if (cachedItems == null)
            throw new Exception($"Error retrieving cached items of type '{typeof(T).Name}'");

        var cachedItem = cachedItems.SingleOrDefault(x => x.Id == item.Id);

        if (cachedItem == null)
        {
            cachedItems.Add(item);
        }
        else
        {
            cachedItems.Remove(cachedItem);
            cachedItems.Add(item);
        }

        Cache.SetItems(cachedItems);

    }

    public static class Cache
    {
        private static object _lock = new object();
        private static DateTimeOffset offset = DateTimeOffset.UtcNow.AddYears(5);

        /// <summary>
        /// Retrieve items of specified type
        /// </summary>
        /// <typeparam name="T"></typeparam>
        /// <returns></returns>
        public static List<T> Items<T>()
        {
            var itemKey = typeof(T).Name;

            lock (_lock)
            {
                return MemoryCache.Default.Get(itemKey) as List<T>;
            }

        }

        /// <summary>
        /// Load items in MemoryCache
        /// </summary>
        /// <typeparam name="T"></typeparam>
        /// <param name="items"></param>
        public static void SetItems<T>(List<T> items)
        {
            var itemKey = typeof(T).Name;

            lock (_lock)
            {
                MemoryCache.Default.AddOrGetExisting(itemKey, items, offset);
            }
        }
    }

}

The MemoryCache is thread safe, all those locks and semaphore are useless.

Also, the Memory Cache by itself makes some serializations so at every update you are deserializing the whole collection of given type, add the item and re-serialize the whole collection.

If you can really use a Dictionary use it, a ConcurrentDictionary < Type,List < T >> will resolve all your problem


Find the best way to get the max sum in a list where sum of three consecutive elements is not allowed I am trying to find a maximum sum of elements in a list where the sum of three consecutive elements is not allowed. So, to find the max sum, you'll have to go through each element sequentially from the first element of the list(Must have first element value in max sum output). You can not select more than two elements in a row.

So, the simple example will be like this:

  Input: list = [1, 2, 3, 4, 5]

  Possible combination of elements might be like this:

  1 = [1, 2, 4, 5] & Sum of elements : 12

  2 = [1, 3, 5] & Sum of elements: 9

  Output: 12 (Elements used to calculate maximum sum: [1, 2, 4, 5])

  Rules followed:

  1. A must selection of first element of the list.

  2. Traverse through list sequentially.

  3. Can not select more than two elements in a row.(Here in this scenario, selected first two element which is 1 & 2, then skip 3 and choose 4 & 5)

And I have got the solution, but I would like suggestions on how to make it more efficient.

These are the details of the problem:

  Input format

  Line 1: A single integer N, the number of elements.

  Line 2: List of elements.

  Output format

  The output consists of a maximum sum of elements

  Sample Input: 1

  5

  [10, 3, 5, 7, 3]

  Sample Output: 1

  23

  (Explanation: 10+3+7+3)

  Sample Input: 2

  8

  [3, 2, 3, 2, 3, 5, 1, 3]

  Sample Output: 2

  17

  (Explanation: 3+3+3+5+3)

  Sample Input: 3

  5

  [10, 1, 5, 7, 3]

  Sample Output: 3

  22

  (Explanation: 10+5+7)

Now, for this problem, I have managed to provide a solution, which is this one

def get_max_sum(num, input):
    output_1 = {'skip': True, 'sum': input[0]+input[1], 0: input[0], 1:input[1]}
    output_2 = {'skip': False, 'sum': input[0]+input[2], 0: input[0], 2:input[2]}

    main_output = [output_1, output_2]

    for i in range(2, num):
        for output in main_output:
            if output.get(i) is None:
                if output['skip']:
                    output['skip'] = False
                    continue
                if len(input) > i+1:
                    if input[i] > input[i+1] or output.get(i-1) is None:
                        output[i] = input[i]
                        output['sum'] += input[i]
                        if output.get(i-1) is not None:
                            output['skip'] = True
                    else:
                        output[i+1] = input[i+1]
                        output['sum'] += input[i+1]
                        if output.get(i) is not None:
                            output['skip'] = True
                else:
                    output[i] = input[i]
                    output['sum'] += input[i]

    if main_output[0]['sum'] >= main_output[1]['sum']:
        return main_output[0]['sum']
    else:
        return main_output[1]['sum']

print(get_max_sum(8, [3, 2, 3, 2, 3, 5, 1, 3]))
print(get_max_sum(5, [10, 3, 5, 7, 3]))
print(get_max_sum(5, [10, 1, 5, 7, 3]))
print(get_max_sum(5, [1, 2, 3, 4, 5]))

And I got the output correct which is this:

17
23
22

Now the thing is, I am looking for a better version of this. I know this is not the most efficient code and I will appreciate any suggestions to improve this code.

This is Code Visualization Link.

EDIT: I have made some minor changes/improvements in my code.

I don't fully understand your code, but it seems to look only at the presence or absence of the previous element when deciding whether to include the current one, so I constructed a test case and verified that it fails:

print(get_max_sum(6, [10, 10, 0, 0, 10, 10]))

should give 40 but actually gives 30.

    if main_output[0]['sum'] >= main_output[1]['sum']:
        return main_output[0]['sum']
    else:
        return main_output[1]['sum']

doesn't need to be so verbose. Prefer

   return max(main_output[0]['sum'], main_output[1]['sum'])

    output_1 = {'skip': True, 'sum': input[0]+input[1], 0: input[0], 1:input[1]}
    output_2 = {'skip': False, 'sum': input[0]+input[2], 0: input[0], 2:input[2]}

What you need to keep track of is the current run length, which can be from 0 (previous element not included) to 2 (both previous elements included, so must skip this one). The requirement to include the first element means that a special case is required at the start. So we get

    best_by_run_length = [float('-inf'), input[0], float('-inf')]
    for elt in input[1:]:
        best_by_run_length = [
            # Run length 0: don't include elt, preceding run length is unconstrained
            max(best_by_run_length),
            # Run length 1: include elt, preceded by run length of 0
            elt + best_by_run_length[0],
            # Run length 2: include elt, preceded by run length of 1
            elt + best_by_run_length[1]
        ]
    return max(best_by_run_length)

print(get_max_sum(8, [3, 2, 3, 2, 3, 5, 1, 3]))
print(get_max_sum(5, [10, 3, 5, 7, 3]))
print(get_max_sum(5, [10, 1, 5, 7, 3]))
print(get_max_sum(5, [1, 2, 3, 4, 5]))

How do you know that the output is correct? Rather than print the result, it's much more useful to print the comparison of the result against the expected value. Or you could take the testing up a level by using doctest (Python 2, Python 3).


Given an integer k and a string s, find the length of the longest substring that contains at most k distinct characters Given an integer k and a string s, find the length of the longest substring that contains at most k distinct characters.

For example, given s = "abcba" and k = 2, the longest substring with k distinct characters is "bcb".

I found few solutions online like. I felt they were bit too complex. I have tried to simply the solution. But it still lacks readability. How can i further improve?

public class DailyCodingProblem13 {
    public static void main(String args[]) {
        String s = "abcba";
        int k = 2;
        int result = solution(s, k);
        System.out.println(result);

        s = "aabacbebebe";
        k = 2;
        result = solution(s, k);
        System.out.println(result);

        s = "aabbcc";
        k = 3;
        result = solution(s, k);
        System.out.println(result);
    }

    static int solution(String s, int k) {
        int n = s.length();
        int max = 0;
        String maxLengthString = null;
        if (n == 0 || k == 0) {
            return max;
        } else {
            StringBuilder window = new StringBuilder();
            Set<Character> set = new HashSet<>();
            for (int i = 0; i < n; i++) {
                Character ch = s.charAt(i);
                // If
                if (set.size() == k && !set.contains(ch)) {
                    // Fetch the last index of the first character at window. Discard the string
                    // before the last index.
                    window = new StringBuilder(
                            window.substring(window.lastIndexOf(Character.toString(window.charAt(0))) + 1));
                    set.clear();
                    for (int j = 0; j < window.length(); j++) {
                        set.add(window.charAt(j));
                    }
                }
                set.add(ch);
                window.append(ch);
                if (window.length() > max) {
                    max = window.length();
                    maxLengthString = window.toString();
                }
            }
        }
        System.out.println("String with max length is " + maxLengthString.toString());
        return max;
    }

}

Note: This is my huumble effort to create a opensource project with best solutions for leetcode problems. This is not for increasing my score in any coding challenges.

Link to my project.

Necessary imports are missing.
The prefix is discarded too aggressively. In a string abaacccc the entire abaa is discarded, and the result becomes 4 instead of 6.
Representing window as a StringBuilder seems excessive. A pair of indices into the original string is enough.


Find if some words in array value are in same array php I have to find where a given string is in my multidimensional array.
So this is my array

$input = array(array('time' => '18:31:00', 'artist' => 'LUIS RODRIGUEZ & DEN HARROW'), array('time' => '18:32:00', 'artist' => 'J BALVIN'), array('time' => '18:33:00', 'artist' => 'THE BLACK EYED PEAS FT J BALVIN'), array('time' => '18:34:00', 'artist' => 'THE BLACK EYED PEAS FT J BALVIN'), array('time' => '18:35:00', 'artist' => 'J BALVIN'));

I have to find at what time I can find this

$artista_inserito = 'THE BLACK EYED PEAS FT J BALVIN';

So I use a function to delimiter my array and even string

function multiexplode($delimiters, $string)
{

    $ready = str_replace($delimiters, $delimiters[0], $string);
    $launch = explode($delimiters[0], $ready);
    return  $launch;
}

// array with string delimeter
$array_delimiter_artisti = array(' FEAT ', ' feat ', ' FT ', ' ft ', ' + ', ' AND ', ' and ', ' E ', ' e ', ' VS ', ' vs ', ' FEAT. ', ' feat. ', ' FT. ', ' ft. ', ' VS. ', ' vs. ', ' , ', ' & ');

// I split the current artist
$artisti_inseriti = multiexplode($array_delimiter_artisti, $artista_inserito);

So I search the given string un my array

foreach ($input as $split) {
    $time = $split['time'];
    $artist = $split['artist'];
    $lista_artisti = multiexplode($array_delimiter_artisti, $artist);

    foreach ($lista_artisti as $nuovo) {
        $artista_split = $nuovo;

        // copy all new data in new array
        $nuovo_array_dati[] = array('time' => $time, 'artist' => $artista_split);
    }
}

foreach ($nuovo_array_dati as $controllo) {
    $time = $controllo['time'];
    $artist = $controllo['artist'];

    foreach ($artisti_inseriti as $trova_artista) {
        $artista_singolo = $trova_artista;

        foreach ($controllo as $index => $a) {
            if (strstr($a, $artista_singolo)) {
                // now I can print where I found the given string $artista_inserito
                echo "$artista_singolo  is at $time ";

            }
        }
    }
}

So this is my output

J BALVIN is at 18:32:00
THE BLACK EYED PEAS is at 18:33:00
J BALVIN is at 18:33:00
THE BLACK EYED PEAS is at 18:34:00
J BALVIN is at 18:34:00
J BALVIN is at 18:35:00

Can I improve this?
Thanks

I have a bias toward regex because I'm generally comfortable with most techniques, so I'll recommend condensing your array of delimiters into a single regex pattern and employ preg_split() to execute the explosion.

After splitting the search string into its separate artists, I am flipping the values & keys to optimize the lookup for later use in the nested loop.

Then as you iterate the haystack of artist times, you can re-use the splitting pattern, then iterate each individual artist and store the desired data for qualifying rows.

Code: (Demo)

$fullArtistTimes = [
    ['time' => '18:31:00', 'artist' => 'LUIS RODRIGUEZ & DEN HARROW'],
    ['time' => '18:32:00', 'artist' => 'J BALVIN'],
    ['time' => '18:33:00', 'artist' => 'THE BLACK EYED PEAS FT J BALVIN'],
    ['time' => '18:34:00', 'artist' => 'THE BLACK EYED PEAS FT J BALVIN'],
    ['time' => '18:35:00', 'artist' => 'J BALVIN']
];

$fullArtist = 'THE BLACK EYED PEAS FT J BALVIN';

$artistDelimiters = '~ (?:[+e,&]|and|f(?:ea)?t\.?|vs\.?) ~i';

$artists = array_flip(preg_split($artistDelimiters, $fullArtist));

$result = [];
foreach ($fullArtistTimes as $row) {
    foreach (preg_split($artistDelimiters, $row['artist']) as $artist) {
        if (isset($artists[$artist])) {
            $result[] = "{$artist} is at {$row['time']}";
        }
    }
}

var_export($result);

Output:

array (
  0 => 'J BALVIN is at 18:32:00',
  1 => 'THE BLACK EYED PEAS is at 18:33:00',
  2 => 'J BALVIN is at 18:33:00',
  3 => 'THE BLACK EYED PEAS is at 18:34:00',
  4 => 'J BALVIN is at 18:34:00',
  5 => 'J BALVIN is at 18:35:00',
)

If this does not work for all of your cases, then I would need to see more test cases and your desired result before I could properly adjust my snippet.


Bank transaction implementation - side-effects of barriers I'm interested in implementing role based code in C++.  I'm also interested in what side effects or barriers (from static strong typing perhaps?) have been added that I am unaware of and if there is a better way.

#include <cstdio>
#include <vector>
#include <string>
#include <numeric>
using namespace std;

// Interfaces to the roles.
class IsourceAccount {
public:
  virtual int availableBalance() = 0;
  virtual void withdraw(int amount) = 0;
};

class IdestAccount {
public:
  virtual int availableBalance() = 0;
  virtual void deposit(int amount) = 0;
};

// MODEL Classes.
class Account: public IsourceAccount, public IdestAccount {
private:
  int balance;
public:
  Account(): balance(0) { }
  int availableBalance() { return balance; }
  void deposit(int amount) { balance += amount; }
  void withdraw(int amount) { balance -= amount; }
};

class Account2: public IsourceAccount, public IdestAccount {
private:
  vector<int> audit;
public:
  Account2() { }
  int availableBalance() { return accumulate( audit.begin(), audit.end(), 0 ); }
  void deposit(int amount) { audit.push_back(amount); }
  void withdraw(int amount) { audit.push_back(-amount); }
};

// How the Model objects interact
class moneyTransferCTX {
  // Role methods (from context and from roleplayers).
  class /*role*/ sourceAccount: public IsourceAccount {
    public:
      void transferTo(moneyTransferCTX *ctx, int amount);
  } *sourceAccount;

  class /*role*/ destAccount: public IdestAccount {
    public:
      void receive(moneyTransferCTX *ctx, int amount);
  } *destAccount;

public:
  moneyTransferCTX(IsourceAccount *src, IdestAccount *dst):
    // Cast the objects (src,dst) to their roles (sourceAccount, destAccount).
    sourceAccount(static_cast<decltype(sourceAccount)>(src)),
    destAccount(static_cast<decltype(destAccount)>(dst))
  { }

  void execute(int amount) {
    this->sourceAccount->transferTo(this, amount);
  }
};

void moneyTransferCTX::sourceAccount::transferTo(moneyTransferCTX *ctx, int amount) {
  if (this->availableBalance() < amount) throw -1;
  this->withdraw(amount);
  ctx->destAccount->receive(ctx, amount);
}

void moneyTransferCTX::destAccount::receive(moneyTransferCTX *ctx, int amount) {
  this->deposit(amount);
}

// Testing...
int main() {
  Account2 *savings = new Account2();
  Account *checking = new Account(); //Changing the class type shouldn't hurt the code.

  moneyTransferCTX *ctx[2];

  savings->deposit(10);
  checking->deposit(100);

  ctx[0] = new moneyTransferCTX(savings, checking);
  ctx[1] = new moneyTransferCTX(checking, savings);

  printf("Opening Status- Savings: %d, Checking: %d\n", savings->availableBalance(), checking->availableBalance());

  try {
    printf("Transfer Savings to Checking 50\n");
    ctx[0]->execute(50);
  } catch(int n) {
    printf("Error: not enough funds\n");
  }

  printf("Savings: %d, Checking %d\n", savings->availableBalance(), checking->availableBalance());

  try {
    printf("Transfer Checking to Savings 30\n");
    ctx[1]->execute(30);
  } catch(int n) {
    printf("Error: not enough funds\n");
  }

  printf("Closing Status- Savings: %d, Checking %d\n", savings->availableBalance(), checking->availableBalance());

  delete ctx[0];
  delete ctx[1];
  delete savings;
  delete checking;
  return 0;
}

In moneyTransferCTX:

You are using pointers. As you never expect these to be NULL (and they can't be changed) you should be using references.

The constructor is casting objects to things that are not necessarily valid.

moneyTransferCTX(IsourceAccount *src, IdestAccount *dst):
  // Cast the objects (src,dst) to their roles (sourceAccount, destAccount).
  sourceAccount(static_cast<decltype(sourceAccount)>(src)),
  destAccount(static_cast<decltype(destAccount)>(dst))
{}

Just because a src is an IsourceAccount does not mean it is also sourceAccount so casting it may not be valid. But it looks like what you are trying was to create a wrapper class (like this):

class sourceAccount
{
  IsourceAccount&  account;
  public:
    sourceAccount(IsourceAccount &acc): account(acc) {}
    void transferTo(moneyTransferCTX& ctx, int amount)
    {
        if (account.availableBalance() < amount) throw -1;
        account.withdraw(amount);

        ctx.destAccount->receive(ctx, amount);
    }
}  sourceAccount;

Personally I think this is overkill and you should just put the work you were putting in these classes into the execute method.

I would do this:

// How the Model objects interact
class moneyTransferCTX
{
  IsourceAccount& sourceAccount;
  IdestAccount&   destAccount;

public:
  moneyTransferCTX(IsourceAccount& src, IdestAccount& dst)
    : sourceAccount(src)
    , destAccount(dst)
  {}

  void execute(int amount)
  {
    if (sourceAccount.availableBalance() < amount) throw -1;
    sourceAccount.withdraw(amount);
    destAccount.deposit(amount);
  }
};

Creating dynamic objects should not be done if not required.

Account2 *savings = new Account2();
Account *checking = new Account(); //Changing the class type shouldn't hurt the code.

It is easier to create normal objects (then you don't have to delete them). If you must create them then you should wrap their usage in a shared pointer.

Account2 savings;
Account  checking; //Changing the class type shouldn't hurt the code.

Learn to use the C++ streams (It provides type safety):

printf("Opening Status- Savings: %d, Checking: %d\n",
       savings->availableBalance(),
       checking->availableBalance());

//

std::cout << "Opening Status- Savings: " << savings->availableBalance()
          << ", Checking: " << checking->availableBalance()
          << "\n";

Your use of exceptions is on the edge of being acceptable. Some people will say it is OK others will say it is in-appropriate. Personally I could go either way depending on what the application is doing. What you should keep in mind that exceptions are used to transfer control when an error occurs but should not normally by used for control flow. Error codes/status can be good internally, but don't leak them outside your interface.

The other thing I would point out is that you should probably not use int (though I suspect you were just doing that as a demonstration) as an exception. Either pick one of the standard exceptions or build your own (derived from std::runtime_error).

  try {
    printf("Transfer Savings to Checking 50\n");
    ctx[0]->execute(50);
  } catch(int n) {
    printf("Error: not enough funds\n");
  }

More pointer comments. Normal (modern C++) contains very few pointers. And even less explicit calls to delete. Any dynamically allocated object should be wrapped inside a smart pointer (or container like object boost::ptr_container springs to mind here). This makes there usage exception safe.

std::auto_ptr<moneyTransferCTX> tr(new moneyTransferCTX(checking, savings));
// Note I am using auto_ptr as this is the only smart pointe in C++03
// But it is not considered a good one (as it is easy to misuse)
// Look up the boost::shared_ptr or if you have C++11 compiler it is part of the standard.

// Note there are several good smart pointers and you should learn when to use
// each one appropriately.


Two variants of a picture database initialization routine I wonder which of the two snippets is better? In the second one, I extracted two short subroutines.

On the one hand, extraction clearly separate the two tasks
On the other hand, it induces 6 more lines for function prototype

The code is about a simple images database called pictdb_file. The first function contributes to the creation of a new database.

/** this is a helper method for [...] blahblah */
static error_enum
db_create_delegate(const char* db_filename, pictdb_file* new_db_file)
{
    strncpy(new_db_file->header.db_name, CAT_TXT,  MAX_DB_NAME);
    new_db_file->header.db_name[MAX_DB_NAME] = '\0';

    new_db_file->header.num_files = 0;
    new_db_file->header.db_version = 0;

    size_t n1;
    n1 = fwrite(&new_db_file->header, sizeof(pictdb_header), 1, new_db_file->fpdb);
    if (n1 != 1) {
        return ERR_IO;
    }

    size_t n2;
    n2 = fwrite(new_db_file->metadata, sizeof(pict_metadata), new_db_file->header.max_files, new_db_file->fpdb);
    if (n2 != new_db_file->header.max_files) {
        return ERR_IO;
    }
    printf("%lu item(s) written\n", n1 + n2);
    return 0;
}

Compared to:

/** helper method [...] blahblah */
static error_enum
db_create_helper(const char* db_filename, pictdb_file* db_file)
{
    finish_initialization(db_file);
    return write_to_file(db_file);
}
static void
finish_initialization(pictdb_file* new_db_file)
{
    strncpy(new_db_file->header.db_name, CAT_TXT,  MAX_DB_NAME);
    new_db_file->header.db_name[MAX_DB_NAME] = '\0';

    new_db_file->header.num_files = 0;
    new_db_file->header.db_version = 0;
}
static error_enum
write_to_file(pictdb_file* db_file)
{
    size_t n1;
    n1 = fwrite(&db_file->header, sizeof(pictdb_header), 1, db_file->fpdb);
    if (n1 != 1) {
        return ERR_IO;
    }
    size_t n2;
    n2 = fwrite(db_file->metadata, sizeof(pict_metadata), db_file->header.max_files, db_file->fpdb);
    if (n2 != db_file->header.max_files) {
        return ERR_IO;
    }
    printf("%lu item(s) written\n", n1 + n2);
    return 0;
}

For me, there's two main things that drive extracting into a function.

Is there scope for reusing the functionality?
Would a function make the code easier to follow?

Looking at your code, it doesn't seem like the functions that you've implemented have much scope for reuse.  Whilst they could make your code easier to follow, I don't think they do, primarily because the names don't really describe what they do.  I find getting names right to be hard work, but well named methods make code a lot easier to follow.

Your method names that you've introduced are:

db_create_helper
    finish_initialization
    write_to_file

They don't tell me anything about what it is you're expecting the methods to do.

An alternative might be something like this:

initialize_db_file
    build_db_header
    write_db_header
    write_db_metadata

These names aren't perfect either, I don't really know what 'metadata' represents and if it should really be part of the header, which is why I've split your write_to_file in to two, however I've tried to name the methods so that they convey what it is I think the methods are doing.  This makes the top level method more meaningful, describing the process of initializing the db file.

As far as method length goes, methods that are too long are a red flag, because they usually mean that too much is being done in one place and the method is likely to be hard to follow.  However, I'm Ok with one line methods, as long as the name for the method actually conveys the intent better than the line it hides.


Replace part of the string with n size with string of m size I wanted to understand memory and pointers better before I endeavour into file path manipulations for larger project. This is simple replace part of the string test function that I need to rewrite into struct to call upon. I used scopes to test for memory leaking and changes, I work on Windows 7, CygWin and notepad++.

I am using std::size_t as an size representation due to:

  A type whose size cannot be represented by std::size_t is ill-formed mentioned by cppreference with "std::size_t is commonly used for array indexing and loop counting".

I decided to go for the c-style null terminated character array since arguments passed through main(int argc, char **argv) are null terminated characters and system calls accept those best. It seemed beneficial to code parts that i need myself instead of relying on another library, and it is tiresome to keep track what is converted with c_str() and what isn't.

As mentioned above, this function is part of project that manipulates, changes and makes full or relative paths to the files. The character array helps me use stat that comes with CygWin to be able to automatically test if path is not accessible, directory or file. I could use std::string but that is an extra header and i plan serious expansions of file paths to include some custom behaviour - which isn't tied to this function.

#include <iostream>

size_t get_str_size(const char* temp){
            size_t s=0;
            while( temp[s] != '\0' )s++;
            return s;
        }

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

    const char * temp = "sublime/subliminal/sfit.exe"; // example string
    std::cout << &temp << "\t" << temp << std::endl; // prints out address and value
    { // wild scope to help me navigate and debug
        const char* shitcake = "sfitcake"; // string to be put in place of sfit
        size_t ldelim = 0;  // / last delimiter
        size_t edelim = 0;  // . end delimiter
        size_t original_size = get_str_size(temp);

        // finding the necessary indexes of charachters
        for( int i =0 ; i <= original_size; i++ ){
                if( temp[i]=='/' ){ldelim=i;}
                if( temp[i]=='.' ){edelim=i;}
            }

        // calculating size that influences the char array
        size_t old_size = edelim - ldelim -1;  // current char array size(-1 to remove last delimiter)
        size_t new_size = get_str_size(shitcake); // new char array size

        // char array duplication , since original string is const
        char* tempcopy = new char[original_size]; // holds the copy of the temp
        for( int i =0 ; i < original_size; i++ ){ tempcopy[i] = temp[i]; }

        // value to be constructed
        char *new_version = new char[ original_size + (new_size-old_size) ];

        // filling the part before replacement happen
        for( int i =0 ; i <=ldelim; i++ )               { new_version[i] = tempcopy[i]; }
        // filling replaced char array
        for( int i =0 ; i <= new_size;i++ )             { new_version[i+1+ldelim] = shitcake[i]; }
        // the rest of original string
        for( int i =edelim ; i < original_size;i++ )    { new_version[i+old_size] = tempcopy[i];}

        //conversion back to const char* with expanded size and new value
        temp = (const char*)new_version;

        //deleting pointers
        delete tempcopy;
        delete new_version;
    }
    std::cout << &temp << "\t" << temp << std::endl;

    return 0;
}

The output i am getting is:

0xffffcbb8  sublime/subliminal/sfit.exe
0xffffcbb8  sublime/subliminal/sfitcake.exe

Which means it works, but i am afraid of pointer not changing memory address even though the size and value changed.

Main concerns and desires for code:

Removing chance of dangling pointer and memory issue
Fast and readable code

There's no definition in scope for size_t.  Since the description implies that you want std::size_t, you'll need

#include <cstddef>
using std::size_t

But I recommend just writing std::size_t in full where you use it - that's much clearer to readers.

size_t get_str_size(const char* temp){
            size_t s=0;
            while( temp[s] != '\0' )s++;
            return s;
        }

You've just re-implemented std::strlen() for no good reason.

    char* tempcopy = new char[original_size]; // holds the copy of the temp

    char *new_version = new char[ original_size + (new_size-old_size) ];

    delete tempcopy;
    delete new_version;

Memory allocated with new[] must be released using delete[] (not delete).  Valgrind will catch this error for you.

We have a memory leak if the first allocation succeeds and the second fails, because there is no delete[] tempcopy if an std::bad_alloc is thrown.

We reference deleted memory here:

    temp = (const char*)new_version;

    delete new_version;

    std::cout << &temp << "\t" << temp << std::endl;

That's Undefined Behaviour (and also caught by Valgrind - I really recommend you run your code under Memcheck, or any similar tool of your choice).

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

Since we never use the arguments, there's no need to give them names; moreover, there's a signature of main() we can use with no arguments:

int main()

     for( int i =0 ; i <= original_size; i++ ){

Avoid comparing signed values (such as i) against unsigned ones (such as original_size).  We probably want the range of i to match, too, so just use the same type for both.

The code isn't very robust against a range of inputs.  In particular, if edelim is less than ldelim, we'll get (unsigned) integer overflow when we compute old_size, giving incorrect results.

Overall, the code looks very much like C code - and poor C code at that (e.g. copying 1 char at a time, rather than with memcpy()).  A C++ implementation (using std::string) is much shorter and more natural.