Create symmetrical matrix from list of values I need to make a matrix (in the form of a numpy array) by taking a list of
parameters of length N and returning an array of dimensions N+1 x N+1 where the off-diagonals are symmetric and each triangle is made up of the values given. The diagonals are equal to 0 - the row sum of the off-diagonals.

I don't really like having to instantiate an empty array, fill the top triangle, then create a new matrix with the bottom triangle filled in. I'm wondering if I can do this in fewer lines (while still being readable) and also wondering if it can be made faster. vals is never going to be very large (between 2 and 100, most likely) but the operation is going to be repeated many times.

def make_sym_matrix(dim, vals):

    my_matrix = np.zeros([dim,dim], dtype=np.double)

    my_matrix[np.triu_indices(dim, k=1)] = vals
    my_matrix = my_matrix + my_matrix.T
    my_matrix[np.diag_indices(dim)] = 0-np.sum(my_matrix, 0)

    return my_matrix

First of all I would use np.zeros() to initialize your matrix. Using np.empty() can create a matrix with large values relative to your values on the diagonal which will affect the computation of 0-np.sum(my_matrix, 0) due to numeric underflow.

For example, just run this loop and you'll see it happen:

for i in xrange(10):
  print make_sym_matrix(4, [1,2,3,4,5,6])

Secondly, you can avoid taking the transpose by re-using the triu indices.
Here is the code combining these two ideas:

def make_sym_matrix(n,vals):
  m = np.zeros([n,n], dtype=np.double)
  xs,ys = np.triu_indices(n,k=1)
  m[xs,ys] = vals
  m[ys,xs] = vals
  m[ np.diag_indices(n) ] = 0 - np.sum(m, 0)
  return m


C# RGBA to ARGB using shifts I came up with the following code to convert a rgba color to an argb color:

public static int ToARGB(int rgba){
    return (rgba << (3*8)) | ((rgba>>8) & 0x00FFFFFF);
}

I used the bitmask 0x00FFFFFF since the rightshift in C# pads with the most significant bit of the number to be shifted. I tested this methode and it seems to work, but I need to be sure I am not missing a case where this is garbage. Any thoughts on my code please?

When I see this question, I immediately understand what you are trying to do as a bitwise rotation (also known as a circular shift). You need to either rotate the bits in the value left by 24 or right by 8 (they yield equivalent results). As the bits are shifted off one end, they're inserted back in the other end.

Ironically, this is one of those rare cases where the code is conceptually simpler in assembly language (!) than it is in C#. The x86 processor family has two very simple instructions that accomplish precisely this feat—ROL and ROR—which rotate left and right, respectively. That means the code in assembly language is almost as simple as:

rol  value, 24

Whereas it is actually more work—and less obvious what is being done—when writing the code in C or C#. You have to explicitly notate both parts of the circular shift. The canonical form is:

public static UInt32 RotateLeft(UInt32 value, UInt32 amount)
{
    Debug.Assert(amount < 32, "Attempted to rotate by more bits than are in the value.");
    return (value << amount) | (value >> (32 - amount));
}

public static UInt32 RotateRight(UInt32 value, UInt32 amount)
{
    Debug.Assert(amount < 32, "Attempted to rotate by more bits than are in the value.");
    return (value >> amount) | (value << (32 - amount));
}

Note that I've explicitly used the UInt32 type here, instead of the short name uint. This is because I've hard-coded assumptions about the type's width, and I like things to be obvious. UInt32 is obviously always a 32-bit type. At least for me, since I bounce back and forth between languages, that's not always so obvious with uint.

I've also thrown in an assertion to check that we're rotating by a reasonable amount. That's just an assertion, though, not an exception, because the C# language standard guarantees that the shift amount will be masked, such that only the lower 5 bits will be used (resulting in a value from 0 to 31). This means that the language protects you from the undefined behavior you would get in C by trying to shift by too many places. (Again, ironically, so does x86 assembly language.)

So this is basically the generalized form of the code that you have. A call like:

argb = RotateLeft(rgba, 24);

would essentially inline to:

argb = (rgba << 24) | (rgba >> 8);

There is one major difference, though. I've written the code to work with unsigned types, rather than signed types. In fact, bitwise operations should always be done on unsigned types because when you do bitwise operations, you are essentially treating the value as a field of bits, rather than as a number. Using an unsigned type is not only conceptually more correct, but also saves you from bugs.

C# automatically differentiates between signed and unsigned shifts (based on the type of the value you're shifting), which is why you found that your code needed that extra mask. The result was being padded with the most significant bit of the number being shifted because that's how a signed shift works—in effect, it does a signed extension, where the sign bit (the most significant bit) is repeated to preserve the sign of the value. (Note that this distinction is only relevant for right shifts; signed and unsigned left shifts are exactly the same.)

But that's something you don't want to have to think about, and something that you wouldn't have to think about if you just follow the rule that bitwise operations imply unsigned types.

So the correct code is precisely what pgs already suggested:

public static UInt32 ToARGB(UInt32 rgba)
{
    return (rgba << (3*8)) | (rgba >> 8);
}

But you might also want to consider Eric Lippert's advice about logical representations. It is merely an implementation detail that these color values are stored as unsigned 32-bit integers. Therefore, it would make sense to wrap that implementation detail up inside of a structure, so that the client never has to deal with the messiness.

Unfortunately, the C# JIT compiler doesn't seem to be able to produce as efficient of code here as a C compiler would. In C or C++, your function would be transformed directly into that assembly-language instruction I mentioned above. In C#, you get 4 instructions instead of that one:

mov  ecx, eax    // make extra copy of value
shl  ecx, 8      // shift that copy left by 8
shr  eax, 24     // shift original right by 24
or   eax, ecx    // combine the two

Oh well. That's still as efficient as you're going to get.


Performing operations on a processed int vector This question came from C++ Primer 5th edition:

  Q: Read a set of integers into a vector. Print the sum of each pair of
  adjacent elements. Change your program so that it prints the sum of
  the first and last elements, followed by the sum of the second and
  second-tolast, and so on.

I was just wondering if there is any way to improve my solution.

Part 1:

int main(){

    vector<int> intVector;
    int number;
    while (cin >> number){
        intVector.push_back(number);
    }
    for (int i = 0; i < intVector.size(); i++){
        int k;
        k = intVector[i] + intVector[i + 1];
        cout << k << endl;
    }
    return 0;
}

Part 2:

int main(){

    vector<int> intVector;
    int number;
    while (cin >> number) {
        intVector.push_back(number);
    }
    int sizeOfIntVector = intVector.size();
    for(int i = 0; i < sizeOfIntVector / 2; i++){
        int k;
        k = intVector[i] + intVector[sizeOfIntVector - 1 - i];
        cout << k << endl;
    }

    return 0;
}

Never use using namespace std;.
Some reasons why here
Part 1 is fine other than the error Bogdan Yarema pointed out. I would personally just start at 1 instead thought.

for (int i = 1; i < intVector.size(); i++){

    int k = intVector[i] + intVector[i - 1];
    cout << k << endl;
}

In case the size of the vector is odd and the element in center should be added to himself (as Bogdan Yarema pointed out as swell), it would be better to get the ceiling of intVector.size() / 2

int half = (intVector.size() + 1 )/ 2;
int last = intVector.size() - 1;
for (int i = 0; i < half; i++) {

    int k = intVector[i] + intVector[last - i];
    std::cout << k << std::endl;
}


ViewModels for image transformations In my project I have a chain of Fragments where each one gets a bitmap, manipulates it, then sends it to another fragment for more processing.

My fragment chain looks like this:

CaptureFragment -> RotateFragment -> CropFragment -> ...

And here's some of the code:

abstract class BaseFragmentInOut<InVM : BitmapViewModel, OutVM : BitmapViewModel>
(private val c1: Class<InVM>, private val c2: Class<OutVM>) : BaseFragment() {
    protected lateinit var inViewModel: InVM
    protected lateinit var outViewModel: OutVM
    fun getInBitmap(): Bitmap = inViewModel.bitmap
    fun setOutBitmap(bmp: Bitmap) { outViewModel.bitmap = bmp }

    @CallSuper
    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        inViewModel = getViewModel(c1)
        outViewModel = getViewModel(c2)
    }
}
sealed class BaseFragment : Fragment() {
   protected fun <T: ViewModel>getViewModel(c: Class<T>): T
            = ViewModelProviders.of(activity)[c]
}

It leads to fairly awkward code like this since we can't have reified generics in classes:

private typealias RVM = RotateViewModel
private typealias CVM = CropViewModel
class CropFragment: BaseFragmentInOut<CVM, RVM>
(CVM::class.java, RVM::class.java) {
...
}

But the alternative isn't that great either, as it leads to a lot of boilerplate code:

class CropFragment: BaseFragment() {
    private lateinit var inViewModel: RotateViewModel
    private lateinit var outViewModel: CropViewModel
    fun getInBitmap(): Bitmap = inViewModel.bitmap
    fun setOutBitmap(bmp: Bitmap) { outViewModel.bitmap = bmp }

    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        inViewModel = getViewModel()
        outViewModel = getViewModel()
    }
}

How good of a practice is my approach and are there other better ways to do this?

kotlinify getViewModel

I would change

protected fun <T: ViewModel>getViewModel(c: Class<T>): T
            = ViewModelProviders.of(activity)[c]
}
//with
protected fun <T: ViewModel>getViewModel(c: KClass<T>): T
            = ViewModelProviders.of(activity)[c.java]
}

after replacing the Class with KClass in the activity, you can use the kotlin ones instead of the java-ones.

then you can use:

private typealias RVM = RotateViewModel
private typealias CVM = CropViewModel
class CropFragment: BaseFragmentInOut<CVM, RVM>(CVM::class, RVM::class) {
    ...
}

second example

In your second example, you tell me you must do everything in the Cropfragment.
This isn't true as your Basefragment can have those fields and the parent can set them.

abstract class BaseFragmentInOut<InVM : BitmapViewModel, OutVM : BitmapViewModel : BaseFragment() {
    protected lateinit var inViewModel: InVM
    protected lateinit var outViewModel: OutVM
    fun getInBitmap(): Bitmap = inViewModel.bitmap
    fun setOutBitmap(bmp: Bitmap) { outViewModel.bitmap = bmp }

    @CallSuper
    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        inViewModel = getViewModel(c1)
        outViewModel = getViewModel(c2)
    }
}

and in your CropFragment:

class CropFragment: BaseFragment() {
    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        inViewModel = getViewModel()   //changes the field in BaseFragment
        outViewModel = getViewModel() //changes the field in BaseFragment
    }
}

template method

If you don't want to have onViewCreated in the basefragment or you want to de other things after having set the viewmodels in onViewCreated in the basefragment, use a template method:

create an abstract method in the super-class that should do what you want... In this case create the ViewModels

abstract class BaseFragmentInOut<InVM : BitmapViewModel, OutVM : BitmapViewModel : BaseFragment() {
    abstract fun createInViewModel() : InVM
    abstract fun createOutViewModel() : OutVM

    @CallSuper
    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        inViewModel = createInViewModel()
        outViewModel = createOutViewModel()
    }
}

your sub-class is then forced to implement them:

class CropFragment: BaseFragment() {
    override fun createInViewModel() = getViewModel()
    override fun createOutViewModel() = getViewModel()
}

to much?

You can of course also combine both the template methods to one called createViewModels.
Then you can create a helper-function in BaseFragmentInOut:

protected inline fun <reified I : InVM, reified O : OutVM>  initialize(){
    inViewModel = getViewModel(I::class)
    outViewModel = getViewModel(O::class)
}

The only thing you have to do at that moment is to implement createViewModels the following way:

override fun createViewModels() = initialize<RVM, CVM>()

At the moment it's not possinle to remove RVM and CVM from the initialize, but it's on the table


Split large file into smaller files, keeping headers and footers I have a folder of SQL files which I need to loop through and:

Determine if any of the files are over 20MB
If so, split them into multiple files, max 20MB filesize

However, there's a complication in that the original large file will have a header and a footer which needs to be included (and slightly tweaked) in each one of the broken down files.

Here is a graphical representation, if it helps. It's based on one file in particular, which is 130MB, with around 1.8m rows

Of course I’m very happy to receive any and all feedback about my code. However, my main issue is that my current code takes around 20 minutes to break this main file into seven smaller counterparts. Therefore, any speed improvements would be fantastic!

Option Explicit

Private Type TFile
    Path As String
    Name As String
    Extension As String
    FullPath As String
    Size As String
    Data() As String
    CurrentBodyPosition As Long
    HeaderStart As Long
    HeaderEnd As Long
    FooterStart As Long
    FooterEnd As Long
End Type
Private File As TFile

Public Sub SplitLargeFiles()
    Dim newFile As String
    Dim i As Long, j As Long, numberOfNewFiles As Long, rowsPerNewFile As Long

    With File
        .HeaderStart = 0 'header always in the same position
        .HeaderEnd = 11
        .CurrentBodyPosition = .HeaderEnd + 1
        .Path = "\\...\"
        .Extension = ".sql"
        .Name = Replace(Dir(.Path & "*" & .Extension), .Extension, "") 'name only, no extension
    End With

    Do While Len(File.Name) > 0
        File.FullPath = File.Path & File.Name & File.Extension
        File.Size = FileLen(File.FullPath) / 1000000 'filesize in MB
        Debug.Print File.Size

        If File.Size >= 20 Then

            With File
                'open file, transfer data to an array and close it
                Open .FullPath For Input As #1
                .Data = Split(input(LOF(1), #1), vbNewLine)
                .Data(0) = Replace(.Data(0), .Name, .Name & "_0") 'specific tweak to data
                Close #1

                'now assign footer positions
                .FooterStart = UBound(.Data) - 5
                .FooterEnd = UBound(.Data)
            End With

            'determine how many files to split the data across, and hence how many rows each new file needs
            numberOfNewFiles = WorksheetFunction.RoundUp(File.Size / 22, 0) '22 gives a buffer over 20
            rowsPerNewFile = (CLng(UBound(File.Data)) - CLng(18)) / numberOfNewFiles

            For i = 1 To numberOfNewFiles
                newFile = File.Path & File.Name & "_" & i & File.Extension
                Open newFile For Output As #2

                'make iterative tweak to first row of header data
                File.Data(0) = Replace(File.Data(0), "_" & (i - 1), "_" & i)

                'transfer header data
                For j = File.HeaderStart To File.HeaderEnd
                    Print #2, File.Data(j)
                Next j

                'transfer body of data
                For j = 1 To rowsPerNewFile
                    If File.CurrentBodyPosition < File.FooterStart Then
                        Print #2, File.Data(File.CurrentBodyPosition)
                        File.CurrentBodyPosition = File.CurrentBodyPosition + 1
                    Else
                        Exit For
                    End If
                Next j

                'transfer footer data
                For j = File.FooterStart To File.FooterEnd
                    Print #2, File.Data(j)
                Next j

                Close #2
            Next i
        End If
        File.Name = Replace(Dir(), File.Extension, "")
    Loop
End Sub

When writing lots of data, it's better to write all of it in one shot wherever possible.

My code only addresses your specific performance issue, e.g. takes too long to run! It lacks the header and footer manipulation needed, however this should be easy to add in at this point. I'd recommend manipulating this in an Array or StringBuilder before ready to write. Overall, your code is clear, just the method employed is non-optimized for this much data.

I separated the directory loop from the split action. It seemed to me finding a qualifying file and splitting the file were separate actions, so it made sense (to me anyway) to split these activities up into separate Subs.

I used utilized the FileSystemObject to make it easier to build a new file path for new files that were split. Also, you had some Magic Numbers I made those into constants, as it is, it's non-obvious what 1000000 is meant to represent.

Feel free to ask questions if something isn't clear, I tried to comment the code as much as possible.

Edit

Made a few tweaks changes with how I'm reading the file in. Seems to be about 10% faster.

Edit 2

For completeness I included (stole?) Thomas Inzina great idea for reading the file line by line. This sped things up considerably. See the revised timings below.

Code

Option Explicit

Public Sub FindFilesToSplit()

    Dim FolderPath As String
    Dim FileNames  As String
    Const FileSizeLimitBytes As Long = 20000000
    FolderPath = "E:\Ex\"

    FileNames = Dir(FolderPath)

    Do While Len(FileNames) > 0
        If (FileLen(FolderPath & FileNames) / FileSizeLimitBytes) > 1 Then SplitFiles (FolderPath & FileNames)
        FileNames = Dir
    Loop

End Sub

Private Sub SplitFiles(ByRef FilePath As String)
    Const BytesToMBs As Long = 1000000
    Const FileSizeThresholdMBs As Long = 20

    Dim TimeRoutine     As Single: TimeRoutine = Timer
    Dim FSO             As FileSystemObject
    Dim FileNumber      As Long
    Dim FileSize        As Long
    Dim FileData()      As String
    Dim NumberOfFiles   As Long
    Dim LinesToRead     As Long
    Dim i               As Long
    Dim j               As Long
    Dim k               As Long
    Dim SplitFileName   As String
    Dim StartingLine    As Long
    Dim EndingLine      As Long
    Dim ChunkofFile     As Variant

    Set FSO = New FileSystemObject
    FileSize = FileLen(FilePath) \ BytesToMBs

    'Compute how many files are going to be needed
    NumberOfFiles = (FileSize \ FileSizeThresholdMBs) + 1

    'Using Thomas Inzina approach here instead, it's a lot faster
    'Awesome stuff Thomas Inzina :)
    FileData = GetTextFileLines(FilePath)

    Debug.Print "Reading and splitting the file took: " & Timer - TimeRoutine & " seconds. The file size is: " & FileSize & " MBs"
    TimeRoutine = Timer

    'Compute the number of lines to read for each iteration
    LinesToRead = (UBound(FileData) \ NumberOfFiles) + 1

    'Use to variables to keep track which lines to read
    'Assumption: each line equally is equal terms of space requirements
    'To be safe you may want to increment LinesToRead by 2 just in case :)
    StartingLine = LBound(FileData)
    EndingLine = LinesToRead

    For i = 1 To NumberOfFiles
        'Resize an array to hold data for a single file
        ReDim ChunkofFile(0 To (EndingLine - StartingLine))

        k = 0
        'Add the text back to a smaller array
        For j = StartingLine To EndingLine
            ChunkofFile(k) = FileData(j)
            k = k + 1
        Next

        'Build the string name for the new file
        'The file name mirrors the parent file, just with an
        'iteration number suffix
        SplitFileName = FSO.GetParentFolderName(FilePath) & "\" & _
                        FSO.GetBaseName(FilePath) & "_" & CStr(i) & _
                        "." & FSO.GetExtensionName(FilePath)

        StartingLine = StartingLine + LinesToRead + 1

        'Adjust ending line to read as dividing by the number of files -
        'won't divide evenly (most of the time)
        If i = NumberOfFiles - 1 Then
            EndingLine = UBound(FileData)
        Else
            EndingLine = EndingLine + LinesToRead + 1
        End If

        'Write the file by joining the array just created
        FileNumber = FreeFile()
        Open SplitFileName For Output Access Write As FileNumber
        Print #FileNumber, Join(ChunkofFile, vbNewLine)
        Close #FileNumber
    Next

    Debug.Print "Rest of Process took: " & Timer - TimeRoutine & " seconds"
End Sub

Private Function GetTextFileLines(ByRef Path As String) As String()
    Const MAX_ROWS  As Long = 30000000
    Dim TextLine    As String
    Dim FileNumber  As Integer
    Dim i           As Long
    Dim TextArray() As String

    ReDim TextArray(MAX_ROWS)

    FileNumber = FreeFile
    Open Path For Input Access Read As #FileNumber

    Do While Not EOF(FileNumber)
        Line Input #FileNumber, TextLine
        TextArray(i) = TextLine
        i = i + 1
    Loop
    Close #FileNumber

    ReDim Preserve TextArray(i - 1)
    GetTextFileLines = TextArray
End Function

Results

The performance is pretty good. I'm processing a ~200MB file into 11 files in about 12 seconds.

Debug Messages:

Reading and splitting the file took: 4.609375 seconds. The file size is: 219 MBs
Rest of Process took: 7.953125 seconds

Edit 3

Let's go for even faster performance! It's been fun trying to make this even faster.

Splitting really slows things down, so I'm ditching it entirely. Instead I'm writing every N number of characters instead. So you do lose the tidy look of split per line with this approach, but the files are still split up equally. It should be possible to keep the lines together by looking ahead/behind to find the linebreak character, I just didn't bother.

I'm using several Win APIs to get a speed boost in a few places. To keep this simple, I did not add the VBA7/win64 API signatures, but this should be easy to do.

Improvements

The Space$() buffer allocation can be somewhat slow when allocating that much buffer, so I replaced with a new approach.

Also, I'm using the CreateFile API too. VBA is a kinda slow writing files, this helped a bit with speed too. All in all, I've got splitting and writing a 130MB file in less than 2 seconds (~1.89 seconds).

Revised Code

Option Explicit

Private Const GENERIC_WRITE = &H40000000
Private Const GENERIC_READ = &H80000000
Private Const FILE_ATTRIBUTE_NORMAL = &H80
Private Const OPEN_ALWAYS = 4
Private Const INVALID_HANDLE_VALUE = -1
Private Const CREATE_ALWAYS = 2
Private Const BytesToMBs As Long = 1000000
Private Const FileSizeThresholdMBs As Long = 20
Private Const FileSizeLimitBytes As Long = 20000000
Private Const FILE_FLAG_WRITE_THROUGH = &H80000000

Private Declare Sub RtlMoveMemory Lib "kernel32" (dst As Any, src As Any, ByVal nBytes As Long)
Private Declare Function SysAllocStringByteLen Lib "oleaut32" (ByVal olestr As Long, ByVal BLen As Long) As Long

Private Declare Function CloseHandle Lib "kernel32" (ByVal hObject As Long) As Long

Private Declare Function WriteFile Lib "kernel32" (ByVal hFile As Long, lpBuffer As Any, _
    ByVal nNumberOfBytesToWrite As Long, _
    lpNumberOfBytesWritten As Long, ByVal lpOverlapped As Long) As Long

Private Declare Function CreateFile Lib "kernel32" Alias "CreateFileA" (ByVal lpFileName As String, _
    ByVal dwDesiredAccess As Long, _
    ByVal dwShareMode As Long, _
    ByVal lpSecurityAttributes As Long, _
    ByVal dwCreationDisposition As Long, _
    ByVal dwFlagsAndAttributes As Long, _
    ByVal hTemplateFile As Long) As Long

Private Declare Function FlushFileBuffers Lib "kernel32" (ByVal hFile As Long) As Long

Private Function AllocString(ByVal Size As Long) As String
    RtlMoveMemory ByVal VarPtr(AllocString), SysAllocStringByteLen(0, Size + Size), 4
End Function

Public Sub FindFilesToSplit()
On Error GoTo ErrorHandler:

    Dim FolderPath As String
    Dim FileNames  As String

    FolderPath = "E:\Ex\"
    FileNames = Dir$(FolderPath)

    Do While Len(FileNames) > 0
        If (FileLen(FolderPath & FileNames) / FileSizeLimitBytes) >= 1 Then SplitFiles (FolderPath & FileNames)
        FileNames = Dir$
    Loop

CleanExit:
    Exit Sub

ErrorHandler:
    Resume CleanExit
End Sub

Private Sub SplitFiles(ByRef FilePath As String)
    Dim TimeRoutine     As Single: TimeRoutine = Timer
    Static FSO          As FileSystemObject
    Dim FileNumber      As Long
    Dim FileData        As String
    Dim NumberOfFiles   As Long
    Dim CharsToRead     As Long
    Dim i               As Long
    Dim SplitFileName   As String
    Dim StartingChar    As Long
    Dim EndingChar      As Long

    If FSO Is Nothing Then Set FSO = New FileSystemObject

    'Compute how many files are going to be needed
    NumberOfFiles = ((FileLen(FilePath) \ BytesToMBs) \ FileSizeThresholdMBs) + 1

    'Get File data
    TimeRoutine = Timer
    FileNumber = FreeFile()
    Open FilePath For Binary Access Read As FileNumber
    FileData = AllocString(LOF(FileNumber))
    Get FileNumber, , FileData
    Close FileNumber

    Debug.Print "Reading and splitting the file took: " & Timer - TimeRoutine & " seconds. The file is 130 mb"
    TimeRoutine = Timer

    'Compute the number of lines to read for each iteration
    CharsToRead = (Len(FileData) \ NumberOfFiles)

    'Counters to keep track of which character we read
    StartingChar = 1
    EndingChar = CharsToRead

    For i = 1 To NumberOfFiles
        'Create the file name
        With FSO
            SplitFileName = .GetParentFolderName(FilePath) & "\" & _
                            .GetBaseName(FilePath) & "_" & CStr(i) & _
                           "." & .GetExtensionName(FilePath)
        End With

        'Write the file
        WriteStringToFile SplitFileName, Mid$(FileData, StartingChar, CharsToRead)
        StartingChar = StartingChar + CharsToRead + 1

        'Adjust ending line to read as dividing by the number of files -
        'won't divide evenly (most of the time)
        EndingChar = IIf(i = NumberOfFiles - 1, Len(FileData), EndingChar + CharsToRead + 1)
    Next

    Debug.Print "Rest of Process took: " & Timer - TimeRoutine & " seconds"
End Sub

Private Sub WriteStringToFile(ByRef FileName As String, _
                              ByRef FileData As String, _
                              Optional NoOverwrite As Boolean = False)

    Dim FileHandle         As Long
    Dim Success            As Long
    Dim BytesWritten       As Long
    Dim BytesToWrite       As Long

    'Quick Fail
    If NoOverwrite = True And Dir$(FileName) <> vbNullString Then Exit Sub

    BytesToWrite = Len(FileData)

    FileHandle = CreateFile(FileName, GENERIC_WRITE Or GENERIC_READ, _
                            0, 0, CREATE_ALWAYS, FILE_FLAG_WRITE_THROUGH, 0)

    If FileHandle <> INVALID_HANDLE_VALUE Then

       Success = WriteFile(FileHandle, ByVal FileData, BytesToWrite, BytesWritten, 0) <> 0

       If Success <> 0 Then
          Success = FlushFileBuffers(FileHandle)
          Success = CloseHandle(FileHandle)
       End If

    End If

End Sub


Parse big JSON for reactive form I implements big reactive form on angular5. This form get data from JSON. I use formBuilder and several nested formGroups.

Every field have default value before rendering form. In console output form values after submit form.

CSS design is very simple.

Live demo here.

js:

  formData = {
    'Информация из КХД': {
      Марка: {
        type: 'input',
        value: 'qwerty',
      },
    'Модель': {
        type: 'select',
        value: ['wqeqwewq', 'sadasdasdda', 'xcvxcxcvxcv',],
        selected: 1,
      },
      'Год выпуска': {
        type: 'input',
        value: '',
      },
    },
    'Общая информация': {
      'ПТС': {
        type: 'input',
        value: 'qwerty',
      },
      'Пробег': {
        type: 'select',
        value: ['tgtrgrt', 'fdvdfvdf', 'csdcscsc',],
        selected: 2,
      },
      'Тип кузова': {
        type: 'select',
        value: ['sdv', '544564564', 'yjyujuyj',],
        selected: 0,
      },
    },
    // and more, more, more...
  }

  form: FormGroup;

  constructor(private fb: FormBuilder) { }

  ngOnInit() {
    this.form = this.fb.group(
      this.createForm(Object.keys(this.formData))
    );
  }

  createForm(fields) {
    const result = {};
    fields.forEach(field => {
      const fields = Object.keys(this.formData[field])
      result[field] = this.fb.group(this.createSubForm(field, fields));
    });
    return result;
  }

  createSubForm(field, fields) {
    const result = {};
    fields.forEach(f => {
      let fd = this.formData[field][f];
      if (fd.type === 'select') {
        result[f] = new FormControl(fd.value[fd.selected]);
      } else if (fd.type === 'input') {
        result[f] = new FormControl(fd.value);
      }
    });
    return result;
  }

  getKeys() {
    return Object.keys(this.form.controls);
  }

  getSubKeys(key) {
    const subControls = this.form.get(key).value;
    return Object.keys(subControls);
  }

  onSubmit() {
    console.log('form submitted', this.form.value);
  }

html:

<form class="form" [formGroup]="form" (ngSubmit)="onSubmit()">

{{key}}

      {{subKey}}

      <input type="text" [formControlName]="subKey" *ngIf="formData[key][subKey].type === 'input'">

      <select [formControlName]="subKey" *ngIf="formData[key][subKey].type === 'select'">
        <option
          *ngFor="let item of formData[key][subKey].value; let i = index"
          [value]="item">
          {{ item }}
        </option>
      </select>

  <button class="submit" type="submit">Submit</button>
</form>

scss:

.head {
    font-weight: bold;
    text-decoration: underline;
    width: 100%;
    margin-bottom: 20px;
}

.key {
    display: flex;
    flex-wrap: wrap;
    justify-content: space-between;
    align-items: flex-start;
    margin-bottom: 30px;

    .subKey {
        width: 47%;
        display: block;
        margin-bottom: 15px;

        >* {
            width: 100%;
        }
    }
}

It's an unusual approach, it's very cryptic and as a result will be a bad maintenance of the code in the future, if in the future you wanna add some custom validators, style or add some dependent validators or other things will became a chimera this component, definetly will kill productivity on quick prototyping. If you wanna handle a big bulks of forms, i suggest you to look at using a dynamic form.


Check existence of a row then update a column not too experienced with SQL, wondering if I can make this better... I am trying to check if a row exists and then add one to the frequency column if it does, if it does not exist it should return false...

The table is indexed by the column in the first select statement

I know about the risks of using formatting with public facing code, but this is for processing on a local machine only

    def check_word(self, word_name):
        self.c.execute('SELECT * FROM {tn} WHERE {cn} = """{wn}"""'.format(tn=self.table1, cn=self.column1, wn=word_name))
        exist = self.c.fetchall()
        if exist:
            new_freq = exist[0][2] + 1
            self.c.execute(
            'UPDATE {tn} SET {c2n}={en} WHERE {c1n}="""{word}"""'.format(tn=self.table1, c2n=self.column2,
                                                                         en=new_freq, c1n=self.column1,
                                                                         word=word_name))
            return True
        return False

Obviously, you already know about SQL injection. So let's forget about that. Ideally, please do defend against it, it's easy enough that it's not worth doing it any other way. Code that is not public facing now, might be next week.

Some other things:

You use fetchall instead of fetchone. fetchall has to load all rows into memory (I assume), which is pretty expensive. Don't use it.

Also, you use SELECT *, which needs to load all columns (of the relevant rows) into memory. Again, expensive.

def check_word(self, word_name):
    self.c.execute('SELECT {c2n} FROM {tn} WHERE {c1n} = """{wn}"""'.format(tn=self.table1, c2n=self.column2, c1n=self.column1, word=word_name))
    data = self.c.fetchone()
    if data is not None:
        new_freq = data[0] + 1
        self.c.execute(
            'UPDATE {tn} SET {c2n}={en} WHERE {c1n}="""{word}"""'.format(tn=self.table1, c2n=self.column2,
                                                                     en=new_freq, c1n=self.column1,
                                                                     word=word_name))
        return True
    return False

Also, there is a bit of a trouble with race-conditions here, but that's probably easily solvable using transactions and locking. Or, better, use 1 query (which is also faster!)

def check_word(self, word_name):
    query = 'UPDATE {table_name} SET {counter} = {counter} + 1 WHERE {word_column} = ?'.format(
        table_name=self.table1,
        counter=self.column2,
        word_column=self.column1,
    )
    self.c.execute(query, word_name)
    return self.c.rowcount > 0

(Here, I use the rowcount attribute of a cursor which returns the number of rows changed in the last query: https://docs.python.org/2/library/sqlite3.html#sqlite3.Cursor.rowcount).

I tried to use more sensible names for the columns, and also used string-interpolation for the values that are hopefully not under user control (and can't be done using parametrized queries anyhow), while using paramatrisation for the word_name which is more likely to change.


Tips on multiple key Map-wrapper I'm creating a simple generic Map-wrapper with multiple keyed values.

I'm intending to use it with storing edges in a graph, where an edge goes from one vertex to another. Thus, I can fetch any of the two vertices in constant time instead of having to loop to find either to/from.

public class MultiMap<K1 extends Object, K2 extends Object, V extends Object> {

    private Map<K1, V> map1;
    private Map<K2, V> map2;

    /**
     * @param map1
     * @param map2
     */
    public MultiMap() {
        this.map1 = new HashMap<K1, V>();
        this.map2 = new HashMap<K2, V>();
    }

    public void put(K1 key1, K2 key2, V value) {
        this.map1.put(key1, value);
        this.map2.put(key2, value);
    }

    ...

    public V removeByKey1(K1 key) {
        V res = this.map1.remove(key);
        K2 k = null;
        for (Map.Entry<K2, V> entry : this.map2.entrySet()) {
            if (entry.getValue() == res) {
                k = entry.getKey();
                break;
            }
        }
        this.map2.remove(k);
        return res;
    }

    public V removeByKey2(K2 key) {
        V res = this.map2.remove(key);
        K1 k = null;
        for (Map.Entry<K1, V> entry : this.map1.entrySet()) {
            if (entry.getValue() == res) {
                k = entry.getKey();
                break;
            }
        }
        this.map1.remove(k);
        return res;
    }

    ...
}

Is there better way of removing? E.g. using LinkedHashMap and then using the key index to do it in constant time instead of linear?

If there's something else not sitting right with you, tell me. All feedback appreciated.

Some notes about the current code:

extends Objects looks unnecessary in the class declaration:

public class MultiMap<K1 extends Object, K2 extends Object, V extends Object> {

I'm not completely sure but the following seems to be the same:

public class MultiMap<K1, K2, V> {

I'd rename res to oldValue or something more descriptive.
Modern IDEs use highlighting to separate local variables from fields, so don't need to use this.. It's rather noise.
Comments like this are just noise:

 /**
 * @param map1
 * @param map2
 */

It doesn't say anything more than the code says. Furthermore, the constructor does not have any parameter like map1 or map2. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)


Searching the Array Index & Element Equality I was asked this question from online coding interview and I have provided my solution that passes all of the test cases. I wanted to see if someone can review my code.

Array Index & Element Equality
Given a sorted array arr of distinct integers, write a function
indexEqualsValueSearch that returns the lowest index i for which
arr[i] == i. Return -1 if there is no such index. Analyze the time and
space complexities of your solution and explain its correctness.
Examples:
input: arr = [-8,0,2,5] output: 2 # since arr[2] == 2
input: arr = [-1,0,3,6] output: -1 # since no index in arr satisfies
arr[i] == i.
[input] array.integer arr [output]: interger

I think you can find this question through this link.
My implementation uses binary search, which gives me \$O(\log(N))\$ time complexity, and space complexity is \$O(1)\$ in my solution.
def index_equals_value_search(arr):
    left = 0
    right = len(arr) - 1

    ind = 0
    last = -1
    while left < right:
        ind = (left + right) // 2
        if arr[ind] - ind < 0:
            left = ind + 1
        elif arr[ind] == ind:
            right = ind - 1
            last = ind
        else:
            right = ind - 1
    if arr[left] == left:
        return left
    return last

Test cases:

Passed 6 Test cases:
Test Cases #1
Input: [0],Expected: 0,Actual: 0
Test Case #2
Input: [0,3],Expected: 0,Actual: 0
Test Case #3
Input: [-8,0,1,3,5],Expected: 3,Actual: 3
Test Case #4
Input: [-5,0,2,3,10,29],Expected: 2,Actual: 2
Test Case #5
Input: [-5,0,3,4,10,18,27],Expected: -1,Actual: -1
Test Case #6
Input: [-6,-5,-4,-1,1,3,5,7],Expected: 7,Actual: 7

I think you've made the right call in going with binary search for this problem. I agree that in terms of asymptotic complexity it is \$ O (log(n)) \$ in time and \$ O(1) \$ in space.

The line ind = 0 is unnecessary because it will be reset 3 lines later before it's ever used.

The implementation seems broadly solid and you have your edge cases covered. I think you could simplify out that last if arr[left] == left: check by changing the loop condition to while left <= right:. That would mean the computer running a couple of unnecessary lines of code going through the loop one last time instead of you writing (and the computer parsing) a couple of unnecessary lines of code checking an edge case. I would probably favour the code uniformity, but there's not much in it.

I think if I was writing that innermost test block I'd do it slightly differently. I'd first check for equality and set last in the case of a match, and then independently (i.e. not in elif) check for which way to search. This just makes it easier to see what you are doing, in terms of record running best and continue search. Something that looks a bit like this

    if arr[ind] == ind:
        last = ind # found a satisfying index. Record it

    if arr[ind] >= ind:
        right = ind - 1 # Navigate left for smallest satisfying index
    else:
        left = ind + 1 # Navigate right for smallest satisfying index

You could possibly do with a couple of comments to help make it clearer what is going on. In particular, I would appreciate a comment explaining what your algorithm is doing at a high level, and where it differs from what I might expect. That is, for example, that it's using binary search, and that it doesn't return immediately on finding a solution because it needs the lowest satisfying number.
Also on code clarity, I find the variable name last confusing. What is it the last of? It could either benefit from a comment saying that it's the best satisfying index found thus far, or renaming to that effect.

if arr[ind] - ind < 0: is a slightly strange and unnecessarily complicated way of writing if arr[ind] < ind:. In python it is normally equivalent because python normally uses arbitrary precision integers, but in some cases where it's using a C style fixed length integer representation it could actually give the wrong answer because of integer underflow.

On a very pedantic note, this answer is potentially incorrect half the time because it ignores negative indicies. Python allows indexing to a[-x] to return the xth entry from the end. If your input array is a=[-7, -2, 5] then a[-2]==-2 and -2 is the lowest such index. I think this is a failure on the part of the question setter, and I'm confident that they don't have it in mind because they asked for a failing return code that would otherwise be a legal return code. Probably a case of "Clarify the requirements" in an interview or development situation rather than "Change the code" but it's worth bearing in mind.


3sum leetcode problem using 2sum Given an array S of n integers, are there elements a, b, c in S such
  that a + b + c = 0? Find all unique triplets in the array which gives
  the sum of zero.

  Note: The solution set must not contain duplicate triplets.

a + b + c = 0

Solution is to just convert this into two pointer technique of solving 2SUM problem. Traverse the array and for each element check if rest of the array has 2 elements that sum up to -a.

class Solution(object):
    def threeSum(self, nums):
        def two_pointer(nums, target):
            '''two pointer technique and it is caller responsibility to pass proper nums'''
            first = 0
            second = len(nums) - 1
            two_sums = []
            while first < second:
                two_sum = nums[first] + nums[second]
                if two_sum < target:
                    first += 1
                elif two_sum > target:
                    second -= 1
                else:
                    two_sums.append([nums[first]] + [nums[second]])
                    while first+1 < len(nums) and nums[first] == nums[first+1]:
                        first += 1
                    while second-1 >= 0 and nums[second] == nums[second-1]:
                        second -= 1
                    first += 1
                    second -= 1
            return two_sums
        """
        :type nums: List[int]
        :rtype: List[List[int]]
        """
        nums = sorted(nums)
        three_sums = []
        i = 0
        while i < len(nums)-2:
            two_sums = two_pointer(nums[i+1:], -nums[i])
            for j in range(len(two_sums)):
                three_sums.append([nums[i]] + two_sums[j])
            while i+1 < len(nums) and nums[i] == nums[i+1]:
                i += 1
            i += 1
        return three_sums

Algorithm

I find one problem here, you create len(nums)-2  times copy of array by calling nums[i+1:]. It's fast but not necessary. In general helps islice iterator to avoid copies of sub-arrays.

Iterators

Lines index_of_list_element += constant in loop strongly suggest rewrite code to use proper iterators to get more python way code. In your code i, first, second are indexes for unique numbers in nums. two_pointer() function also can be iterator to avoid create temporary list inside main loop.

Code

Idea of code with iterators

def three_sum(nums):
    def uniques(nums_sorted, start, stop, step):
        prev = None
        for idx in range(start, stop, step):  # xrange() in python2
            value = nums_sorted[idx]
            if value != prev:
                prev = value
                yield idx, value

    def two_pointer(nums_sorted, neg_a, start):
        biter = uniques(nums_sorted, start, len(nums_sorted), 1)
        citer = uniques(nums_sorted, len(nums_sorted) - 1, start, -1)
        (bidx, b), (cidx, c) = next(biter), next(citer)
        while bidx < cidx:
            two_sum = b + c
            if two_sum == neg_a:
                yield b, c
            if two_sum <= neg_a:
                bidx, b = next(biter)
            if two_sum >= neg_a:
                cidx, c = next(citer)

    nums = sorted(nums)
    return [[a, b, c] for aidx, a in uniques(nums, 0, len(nums) - 2, 1)
                      for b, c in two_pointer(nums, -a, aidx + 1)]


Quicksort for a pseudosite I have a pseudosite, where I actually post small functions, so that I can re-use them*, but some posts have visitors. The top is Quicksort (C++). I feel that beginners visit it, so I do not care in improving (its poor) performance, but only the readability, so that the beginner can catch things more easily.

#include <iostream>

void quickSort(int a[], int first, int last);
int pivot(int a[], int first, int last);
void swap(int& a, int& b);
void swapNoTemp(int& a, int& b);
void print(int array[], const int& N);

using namespace std;

int main()
{
    int test[] = { 7, -13, 1, 3, 10, 5, 2, 4 };
    int N = sizeof(test)/sizeof(int);

    cout << "Size of test array :"  << N << endl;

    cout << "Before sorting : " << endl;
    print(test, N);

    quickSort(test, 0, N-1);

    cout << endl << endl << "After sorting : " << endl;
    print(test, N);

    return 0;
}

/**
 * Quicksort.
 * @param a - The array to be sorted.
 * @param first - The start of the sequence to be sorted.
 * @param last - The end of the sequence to be sorted.
*/
void quickSort( int a[], int first, int last )
{
    int pivotElement;

    if(first < last)
    {
        pivotElement = pivot(a, first, last);
        quickSort(a, first, pivotElement-1);
        quickSort(a, pivotElement+1, last);
    }
}

/**
 * Find and return the index of pivot element.
 * @param a - The array.
 * @param first - The start of the sequence.
 * @param last - The end of the sequence.
 * @return - the pivot element
*/
int pivot(int a[], int first, int last)
{
    int  p = first;
    int pivotElement = a[first];

    for(int i = first+1 ; i <= last ; i++)
    {
        /* If you want to sort the list in the other order, change "<=" to ">" */
        if(a[i] <= pivotElement)
        {
            p++;
            swap(a[i], a[p]);
        }
    }

    swap(a[p], a[first]);

    return p;
}

/**
 * Swap the parameters.
 * @param a - The first parameter.
 * @param b - The second parameter.
*/
void swap(int& a, int& b)
{
    int temp = a;
    a = b;
    b = temp;
}

/**
 * Swap the parameters without a temp variable.
 * Warning! Prone to overflow/underflow.
 * @param a - The first parameter.
 * @param b - The second parameter.
*/
void swapNoTemp(int& a, int& b)
{
    a -= b;
    b += a;// b gets the original value of a
    a = (b - a);// a gets the original value of b
}

/**
 * Print an array.
 * @param a - The array.
 * @param N - The size of the array.
*/
void print(int a[], const int& N)
{
    for(int i = 0 ; i < N ; i++)
        cout << "array[" << i << "] = " << a[i] << endl;
}

As you see, I use many things from C, since I wanted the code to be able to work in a C program with minor modifications, thus I am adding c tag too.

*since I find it easier to find them there than in my FS and it helps in restoring harmony after a nuke.

Only small/minor issues:

Since code it to be portable to C with minor mods, suggest demarcating that which is sort code from test code.  Its appears the sort code is only these 3.  Do not mix test code with the application code.  Better in separate files.

void quickSort( int a[], int first, int last )
int pivot(int a[], int first, int last)
void swap(int& a, int& b)

pivot(), swap() should be static functions.  No need for them outside this file.
Minor: int vs. size_t.  An int index may lack sufficient range to index an array.  Use size_t as that is Goldilocks type neither to narrow nor wide a type to handle all array sizes - it is just right.  As size_t is some unsigned type, watch out for attempting to create negative values.  I did not notice any issue for your code concerning that.

// int N = sizeof(test)/sizeof(int);
size_t N = sizeof(test)/sizeof(int);

// void quickSort( int a[], int first, int last )
void quickSort( int a[], size_t first, size_t last )

// int pivot(int a[], int first, int last)
size_t pivot(int a[], size_t first, size_t last)

The sort would works well with other types beside int.   Perhaps code that way.

typedef int sort_type;
void quickSort(sort_type a[], size_t first, size_t last )

Function signature: Rather than oblige the calling code to supply 0, n-1, create a top level call:

void quickSortTop(sort_type a[], size_t size) {
  if (size) quickSort(a, 0, size-1);
}

Minor: Keep local variables as local as able.  pivotElement could be declared and initialized in 1 step local to the if() block as it is not used outside the block.  This borders on style issues, but as a rule, limiting variable scope is easier to see its use and impact.

int pivotElement;  // here
if(first < last) {
    int pivotElement = pivot(a, first, last); // or here
    quickSort(a, first, pivotElement-1);
    quickSort(a, pivotElement+1, last);
}

Minor: Correct comment?  I would think to reverse the order a >= would be needed.  If this is not so, then I would expect < to work slightly faster than <=.

/* If you want to sort the list in the other order, change "<=" to ">" */
/* If you want to sort the list in the other order, change "<=" to ">=" */

Minor: Dead code swapNoTemp().  No explanation for its existance here.


Find the combination of matches which are closest to each other Problem - Given three sorted arrays find combinations which are closest to each other.

example -

i/p - 3,8,18  7,11,16  10,15,19
o/p - (8,7,10)

i/p - 2,2,6  11,15,15  8,8,18
o/p - (6,11,8)

Kindly review this scala code and suggest improvements

import scala.math.Ordering._

object NearestMatch {

  def main(args: Array[String]) = {
    val size = args(0).toInt
    val range = args(1).toInt
    val (a,b,c) = (Array.fill(size)(scala.util.Random.nextInt(range)).sorted,
           Array.fill(size)(scala.util.Random.nextInt(range)).sorted,
           Array.fill(size)(scala.util.Random.nextInt(range)).sorted)

   println(a.mkString(","))
   println(b.mkString(","))
   println(c.mkString(","))

  var (i,j,k) = (0,0,0)
  var (pa,pb,pc) = (0,0,0)

  var curDiff = 12345 // a very large number
  while (i < a.size && j < b.size && k < c.size) {
    val min = Math.min(a(i), Math.min(b(j), c(k)))
    val max = Math.max(a(i), Math.max(b(j), c(k)))
    val newDiff = max-min

    if (curDiff > newDiff) {
      pa = i
      pb = j
      pc = k
      curDiff = newDiff
    }

    if (a(i) == min) i+=1
    else if (b(j) == min) j+=1
    else k+=1
  }
   println(a(pa), b(pb), c(pc))
  }
}

A few things I noticed while looking over the code:

You don't need the import scala.math.Ordering._ statement. It's not buying you anything.
It would be easier to read/follow the code if you used better variable names.
12345 actually isn't a very big Int. Int.MaxValue would be better.
If this val size = args(0).toInt is zero then this println(a(pa), b(pb), c(pc)) will throw.
According to a hint from the IntelliJ IDE, array.length is more efficient than array.size because .size "requires an additional implicit conversion to SeqLike to be made."

But the real problem is that you're using Scala, a functional language, to write imperative code with many mutable variables, making the code more verbose.

Your algorithm is efficient and limits the number of while loop iterations, so how to express it in a functional manner: recursion.

def minDif[N:Numeric](x :Seq[N] ,y :Seq[N] ,z :Seq[N]
                     ,curSet :Seq[N] ,curDiff :N) :Unit = {
  import Numeric.Implicits._
  import Ordering.Implicits._

  if (x.isEmpty || y.isEmpty || z.isEmpty)
    println(curSet.mkString(","))  //done
  else {
    val newSet  = Seq(x.head, y.head, z.head)
    val newMin  = newSet.min
    val newDiff = newSet.max - newMin
    val (nxtSet, nxtDiff) = if (curDiff > newDiff) (newSet, newDiff)
                            else                   (curSet, curDiff)
    newSet match {
      case Seq(`newMin`,_,_) => minDif(x.tail, y, z, nxtSet, nxtDiff)
      case Seq(_,`newMin`,_) => minDif(x, y.tail, z, nxtSet, nxtDiff)
      case Seq(_,_,`newMin`) => minDif(x, y, z.tail, nxtSet, nxtDiff)
    }
  }
}

// a b and c have already been populated and sorted
minDif(a, b, c, Seq(), Int.MaxValue)

The minDif() method is tail recursive so it is compiled to an equivalent while loop under the hood.

Notice that I used Seq as the collection type. This allows minDif() to accept many different types as input: Array, Stream, Vector, etc. Since there is no indexing into any collection there is little advantage in restricting it to just arrays.

Also the element type is Numeric so this will work with Int, Float, Long, etc.


A queue with access to the last element Following this question, I realized there is no built-in data structure like that, so I implement my own queue using List<T>.

Please comment on performance (is there any better way instead of using List<T>?)

public class MyQueue<T>
{
    private List<T> _items;
    private int index;

    public int Count
    {
        get
        {
            return _items.Count;
        }
    }

    public T Last
    {
        get
        {
            return _items[index];
        }
    }

    public MyQueue()
    {
        index = -1;
        _items = new List<T>();
    }

    public void Enqueue(T item)
    {
        _items.Add(item);
        index++;
    }

    public T Dequeue()
    {
        T i = _items[0];
        _items.RemoveAt(0);
        index--;
        return i;
    }

    public T Peek()
    {
        return _items[0];
    }
}

In general, when trying to reimplement basic collections like this, you're going to have a lot of difficulty getting near the performance of the in-built .NET collections. As others have said, removing frequently from the beginning of a List<T> has very poor performance. However, just looking at the circular buffer article already linked, my immediate reaction is "yeesh, that is not complexity I'd want to add to my code!" Especially when I know somebody's already done it for me in the existing Queue<T>!

So in situations like this, the answer is usually to either be willing to sacrifice performance, or to try to make use of the actual collection as much as possible. In fact, all you want is a queue that has a little extra functionality, so why not do just that, using composition? There's no IQueue<T> interface in .NET, so we can't implement the decorator pattern, but we can get close:

class MyQueue<T>
{
    private readonly Queue<T> _inner = new Queue<T>();

    public int Count
    {
        get
        {
            return _inner.Count;
        }
    }

    public T Last { get; private set; }

    public void Enqueue(T item)
    {
        _inner.Enqueue(item);
        Last = item;
    }
    //...
}

In more general review terms, the code you wrote looks good. A few minor points:

index and _items follow different naming schemes.
MyQueue is not a good class name. Try to pick a name that actually describes the class
As firda said, you shouldn't try to keep track of the index yourself, it's just Count - 1. You're tracking the same thing in two different ways.
You may want to consider implementing IEnumerable<T>. Using iterator methods, this wouldn't be too complicated, and it'd allow the full power of LINQ to be used with your class. However, I would definitely file this under YAGNI until it actually comes up as something you find yourself wanting, unless your intention with this code is to put it in a public library for others to use.


Simple way to loop through a JavaScript object array? I have an object array and i am using gulp 4.
libs:
    {
        a: {
          js: [
            {
              src: 'path/from/a1.js',
              dest: 'path/to/a1.js',,
            },
            {
              src: 'path/from/a2.js',
              dest: 'path/to/a2.js',,
            },
          ],
          css: [
            {
              src: 'path/from/a1.css',
              dest: 'path/to/b1.css',,
            },
          ],
        },
        b: {
          js: [
            {
              src: 'path/from/b.js',
              dest: 'path/to/b.js',,
            },
          ],
        },
      }

I need to know all the src and dest values so that I can move files from src to dest.
const moveLibs = (done) => {
  Object.entries(libs).forEach(([key, value]) => {
    const types = value;

    Object.entries(types).forEach(([key, value]) => {
      const srcAndDest = value;

      Object.entries(srcAndDest).forEach(([key, value]) => {
        return gulp
          .src(value.src)
          .pipe(gulp.dest(value.dest));
      });
    });
  });

  done();
};

This method is successful, but I feel that it is not simple enough, please tell me a simpler method, thank you.

A short review;

I prefer to use the function keyword
You are using libs like a global, I would pass it in the function
If you don't need both key and value, then you can just use Object.values
Building on that, you might as well use a properly named function parameter and cut the lines in two

function moveLibs(libs, done){
  Object.values(libs).forEach(types => {
    Object.values(types).forEach(srcAndDest  => {
      Object.values(srcAndDest).forEach(value => {
        console.log(`Piped ${value.src} to ${value.dest}`);
      });
    });
  });

  done();
};

libs=
    {
        a: {
          js: [
            {
              src: 'path/from/a1.js',
              dest: 'path/to/a1.js',
            },
            {
              src: 'path/from/a2.js',
              dest: 'path/to/a2.js',
            },
          ],
          css: [
            {
              src: 'path/from/a1.css',
              dest: 'path/to/b1.css',
            },
          ],
        },
        b: {
          js: [
            {
              src: 'path/from/b.js',
              dest: 'path/to/b.js',
            },
          ],
        },
      };

moveLibs(libs, ()=>console.log("Hello World!"));


Simple file recurser I'm using C# for a project and thought I would get a feel for the language by making some simple programs.  My current program is a simple file recurser that prints the names of files and directories.  Any suggestions would be greatly appreciated.

using System;
using System.IO;

namespace FileRecurser
{
    class Program
    {
        private static int indent = -2; // Start off at -2 so first indent
                                        // starts at 0.

        static void Main(string[] args)
        {
            if (args.Length != 1)
            {
                Console.WriteLine("Usage: recurser <directory>");
                return;
            }

            // Check if directory exists.
            string directory = args[0];
            if (!Directory.Exists(directory))
            {
                Console.WriteLine("Error: {0} doesn't exist!", directory);
                return;
            }

            Console.WriteLine("Printing files for {0}\n", directory);
            PrintDirectoryFiles(directory);
        }

        private static void PrintDirectoryFiles(string directory)
        {
            indent += 2;

            // If any subdirectories.
            string[] subdirectories = Directory.GetDirectories(directory);

            if (subdirectories.Length != 0)
            {
                foreach (var subdirectory in subdirectories)
                {
                    // Change color to reflect directory.
                    Console.ForegroundColor = ConsoleColor.Cyan;

                    Console.WriteLine(
                        "{0}{1}:",
                        new String(' ', indent),
                        Path.GetFileName(subdirectory)
                    );

                    Console.ResetColor();

                    PrintDirectoryFiles(subdirectory);
                    Console.WriteLine();
                }
            }

            // Print file names.
            string[] files = Directory.GetFiles(directory);

            if (files.Length != 0)
            {
                foreach (var file in files)
                {
                    Console.WriteLine(
                        "{0}{1}",
                        new String(' ', indent),
                        Path.GetFileName(file)
                    );
                }
            }

            indent -= 2;
        }
    }
}

I don't like how you use static field to indicate level of indentation, I think it would be better if you passed it down the recursion in parameters of the methods.

And while you're at it, you should probably separate your concerns: don't have everything in your Program class, but have one class that recurses directories, and one that writes them out. That way, you can reuse them if you want to write the output in a different way (say in a GUI app) or if you want to recurse through something else than files (say, registry entries).

Also, there is no need to check whether a collection is empty before iterating it, it just unnecessarily clutters the code.

And you should think about handling exceptions: there might be directories you don't have access to. And there's also a chance that a directory was deleted while you were iterating its siblings, so you should handle that too.


Implement a stack using a linked list I am to implement a stack in C using a linked list. My code is below. My design:

I use stack_ prefix to each function, and each function has a Stack* self pointer. This is similar to object-oriented languages’ bundling of data and methods. I do not know how idiomatic this is in C.
Each function returns a bool indicating success or failure. A more detailed implementation would have error codes and messages. I do not know how idiomatic this is in C.
The stack_init function does not call malloc, so that one could define an instance of Stack locally, as in the test methods below.
I used int as the type of data, but this could be modified for other types. I did not use void* since void* would lose type safety.

stack.h

#ifndef STACK_H_
#define STACK_H_

#include <stdbool.h>

typedef struct Node {
  int data;
  struct Node* next;
} Node;

typedef struct {
    Node* root;
} Stack;

bool stack_init(Stack* self);
bool stack_delete(Stack* self);
bool stack_pop(Stack* self, int* data);
bool stack_push(Stack* self, int data);

#endif  // STACK_H_

stack.c

#include <stdlib.h>
#include "stack.h"

bool stack_init(Stack* self) {
  if (self == NULL) {
    return false;
  }

  self->root = NULL;
  return true;
}

bool stack_delete(Stack* self) {
  Node* next;
  while (self->root != NULL) {
    next = self->root->next;
    free(self->root);
    self->root = next;
  }

  return true;
}

bool stack_push(Stack* self, int data) {
  if (self == NULL) {
    return false;
  }

  Node* node = malloc(sizeof(Node));
  if (node == NULL) {
    return false;
  }

  node->data = data;
  node->next = self->root;
  self->root = node;

  return true;
}

bool stack_pop(Stack* self, int* data){
  Node* root = self->root;
  if (root == NULL) {
    return false;
  }

  *data = root->data;
  self->root = root->next;
  free(root);

  return true;
}

stack_test.c

#include <assert.h>
#include "stack.h"

static void test_null_stack_should_fail_to_init() {
  // arrange
  Stack* stack = NULL;

  // act
  assert(!stack_init(stack));
}

static void test_empty_stack() {
  // arrange
  Stack stack;

  // act
  assert(stack_init(&stack));
  assert(stack_delete(&stack));
}

static void test_push_then_pop() {
  // arrange
  Stack stack;
  stack_init(&stack);
  stack_push(&stack, 1);
  stack_push(&stack, 2);
  stack_push(&stack, 3);
  int data;

  // act
  assert(stack_pop(&stack, &data));
  assert(data == 3);
  assert(stack_pop(&stack, &data));
  assert(data == 2);
  assert(stack_pop(&stack, &data));
  assert(data == 1);
  assert(!stack_pop(&stack, &data));
  assert(stack_delete(&stack));
}

static void test_push() {
  // arrange
  Stack stack;
  stack_init(&stack);
  stack_push(&stack, 1);
  stack_push(&stack, 2);
  stack_push(&stack, 3);

  // act
  assert(stack_delete(&stack));
}

int main() {
  test_null_stack_should_fail_to_init();
  test_empty_stack();
  test_push_then_pop();
  test_push();
}

stack_delete only returns true. If a return value from a function carries no information, better make it void.
There is no consistency in testing parameters. Some functions test for self not being null, other directly proceed to calculating self->root.
The Stack existence is only justifiable if it has more data members than just root. As @glampert mentioned in comments, tracking size is one of the candidates. On the other hand,  abandoning Stack for good, and letting client manage the root, as in

Node * stack_push(Node * root, data);
....

root = stack_push(root, data);

is also a viable strategy.
It would be nice to provide a non-destructive top() interface to access the root data. There is even a popular belief that a pop-like function should only pop an element from the top of stack (as opposed to pop element and return data); in this school of thought top() is a must.


Defining a certain member function Consider the following:

#include <iostream>

struct State { virtual ~State() = default; };

struct Drunk : State {
    void singWhileDrunk() {std::cout << "Singing while drunk.\n";}
};

struct Person {
    State* state;
    void singWhileDrunk() {dynamic_cast<Drunk*>(state)->singWhileDrunk();}  // Is this good?
};

int main() {
    Person bob;
    bob.state = new Drunk;
    bob.singWhileDrunk();
    // dynamic_cast<Drunk*>(bob.state)->singWhileDrunk();  // Using this is better?
}

What I wonder is if Person::singWhileDrunk() should really be defined in Person or not.  singWhileDrunk() only has true meaning if the person is drunk, so to define it in Person seems wrong to me. However, it does simplify the code in main(), especially if it is to be used a lot.

(dynamic_cast<Drunk*>(bob.state)->singWhileDrunk();

is clearly more typing (and may run into difficulties if I want to redefine it everywhere it is used, e.g. change dynamic_cast to static_cast).  Another issue I have is that in my program I have many different types of states, each with their own special functions, and to define them all in Person will really bloat the Person class with MANY, MANY functions that don't even seem to belong in Person.  So there seems to be pros and cons to both choices and would like to hear what others have to say about this.

This is just an example of course. In reality, I have states like FlySpellState, with the function flies(), which also seems to have no place in Person (since people cannot fly normally), though it could be.

You should never really be doing any manual casting.
Virtual functions usually provide you with the functionality you are looking for.

How about:

struct State
{
    virtual void sing() = 0;
};

struct Drunk: State
{
    virtual void sing() { std::cout << "Singing while drunk.\n"}
}

struct Sober: State
{
    virtual void sing() { std::cout << "Singing Opera.\n"}
}

class Person
{
     std::auto_ptr<State>   state;
     public:
         Person()
            : state(new Sober)
         {}
         void sing()
         {
             state->sing();
         }
         void drink()
         {
             state.reset(new Drunk);
         }
         void sleep()
         {
             state.reset(new Sober);
         }
};

Then your code in main is even more trivial:

int main() {
    Person   bob;
    bob.drink();  // Don't allow manual changes in state.
                  // Provide actions that internally modify the state of your object.
    bob.sing();
    bob.sleep();
    bob.sing();
}

  This is just an example of course. In reality, I have states like FlySpellState, with the function flies(), which also seems to have no place in Person (since people cannot fly normally), though it could be.

Sure. So what you want is a bunch of different states. These states can all be changed when diffent situations apply.

struct Action
{
    virtual void doAction()  {std::cout << "Nothing Happens\n";}
};

class Person
{

    std::map<std::string, std::unique_ptr<Action>>  actionMap;
    Action& getAction(std::string const& action)
    {
        static Action defaultAction;    // Will do Nothing by default.

        auto find = actionMap.find(action);
        return find == actionMap.end()
                  ? defaultAction
                  : *(find->second);
    }

    void sing()
    {
        getAction("sing").doAction();
    }
    void fly()
    {
        getAction("fly").doAction();  // Default do nothing unless you have read the scroll of flying
    }
    void drink()
    {
        actionMap["sing"].reset(new Drunk);
        actionMap["walk"].reset(new Stumble);
        actionMap["talk"].reset(new Slur);
    }
    void readScrollOfFlying()
    {
        actionMap["fly"].reset(new FlySpellState);
    }
};

Rather than using std::unique_ptr and lots of calls to new. You may want to look up the flyweight pattern.


Create a complete binary tree and do level order traversal on it How can I optimize this code?

#include<iostream>
#include<queue>
using namespace std;

struct Node
{
    int data;
    Node *left,*right;
};

Node *createNode(int data){
    Node *node=new Node;
    node->left=NULL;
    node->right=NULL;
    node->data=data;
    return node;
}

Node buildTree(Node *newNode,queue<Node *>&q){
    Node *f=q.front();
    if(f->left && f->right) {q.pop();f=q.front();}
    if(!f->left) f->left=newNode;
    else if(!f->right) f->right=newNode;
    q.push(newNode);
}

void levelOrder(Node *root){
    queue<Node*>q;
    if(root){
        q.push(root);
        while(!q.empty()){
            Node *d =q.front();
            q.pop();
            cout<<d->data<<" ";
            if(d->left) q.push(d->left);
            if(d->right) q.push(d->right);
        }
    }
}

int main(){
    Node *root=createNode(10);
    queue<Node *>q;
    q.push(root);
    for(int i=0;i<10;i++){
        buildTree(createNode(i),q);
    }
    levelOrder(root);
    return 0;
}

using namespace std;

using namespace std;

Why is “using namespace std;” considered bad practice?

The particular problem I have with it here is that it obscures that you are using std::queue and not some custom class.

Note that this can also cause problems if you later convert it to header file, which would be natural for a general use data structure.

Don't go the long way around

Node *createNode(int data){
    Node *node=new Node;
    node->left=NULL;
    node->right=NULL;
    node->data=data;
    return node;
}

Rather than create a function for this, consider just creating a constructor for struct Node.

Node::Node(int datum) {
    this->datum = datum;
    left = NULL;
    right = NULL;
}

Note that datum is the singular form of data, so I changed the field in Node to datum as it only holds one piece.

        buildTree(createNode(i),q);

could become

        buildTree(new Node(i), q);

This calls a constructor directly rather than creating a function just to call the default constructor and set the data.

Why wait?

    if(f->left && f->right) {q.pop();f=q.front();}
    if(!f->left) f->left=newNode;
    else if(!f->right) f->right=newNode;

This could be

    if (f->left) {
        f->right = newNode;
        q.pop();
    } else {
        f->left = newNode;
    }

This works because you know that something is only in the queue until both its children are added.  Then it is removed.  So you don't need to check for the both children full case if you only ever add to the tree with this method.

I changed the order because I find it easier to read positive conditions with else blocks.  An else is essentially the negation of the if.  This way we aren't negating a negative.

What if something else adds to the tree?  That would be problematic.  Of course, that was already true.  Something else adding to the tree would interfere with the order that you use the queue to enforce.

Removals are also problematic with either implementation.

This is why Node would normally be a hidden data type, not exposed to callers.  Then you could be sure that only your tree class would be able to access it.

Don't make data structures that you don't use

    queue<Node*>q;
    if(root){

If you do this in the other order, you can save the overhead of creating the std::queue.

    if (root) {
        queue<Node*> q;

or even

    if (!root) {
        return;
    }

    queue<Node*> q;

Which also reduces the indent.

Efficiency

The biggest problem with efficiency that I see is that this doesn't seem to be a tree in any meaningful sense.  Sure, the data structure behind it is a tree, but the operations aren't.  You buildTree in insertion order.  Then you levelOrder to read back the tree in insertion order.  If you only wanted to do that, then a std::vector would be a much more efficient data structure.  You wouldn't need the std::queue to manage the insertions nor reading them back.

    Node *root=createNode(10);
    queue<Node *>q;
    q.push(root);
    for(int i=0;i<10;i++){
        buildTree(createNode(i),q);
    }
    levelOrder(root);

would become

    std::vector<int> data;
    data.push_back(10);
    for (int i = 0; i < 10; i++) {
        data.push_back(i);
    }

    for (auto datum : data) {
        std::cout << datum << " ";
    }

Isn't that simpler?  No new functions required.  Also, rather than using a pointer-based data structure and a queue, this just uses one array-based data structure.  You could also use a std::queue instead of a std::vector.

Encapsulation

This is also encapsulated better.  Instead of having to create a tree structure and a queue in order to call buildTree, this simply calls push_back and lets std::vector handle the details internally.  Consider the following interface

class InsertionOrderTree {
    struct Node {
        int datum;
        Node *left;
        Node *right;
    };

    Node *root;
    std::queue<int> q;

    public:
    InsertionOrderTree();
    build(int datum);
    traverse();
};

InsertionOrderTree::InsertionOrderTree() {
    root = NULL;
}

This hides the implementation detail of using a queue from any callers.  Callers shouldn't have to know how you do things, just what you promise to do.  They certainly should not have to create a specific data structure just to help you do whatever it is that you are doing.  You should create your own data structures if you need them.

Note that the implementations of build and traverse would change from your original versions.  In particular, build needs to handle a NULL root.

Unsolicited advice

This is a bad use case for a tree though.  You should probably either find a different use case that better fits a tree (if you just want to implement a tree) or use a different data structure (if this is really the problem that you need to solve).


How does this Binary Search Tree look? I wrote a BST in C a while back and may use it at some point.

 search_tree tree_make_empty( search_tree tree )
 {
   if ( tree != NULL )
   {
       tree_make_empty( tree->left );
       tree_make_empty( tree->right );
       free( tree );
   }
   return NULL;
 }

 tree_position tree_find( CHAR_DATA *target, search_tree tree )
 {
     if ( tree == NULL )
       return NULL;

     if ( target < tree->hatedata->target_char )
       return tree_find( target, tree->left );
     else if ( target > tree->hatedata->target_char )
       return tree_find( target, tree->right );
     else
       return tree;
 }

 search_tree tree_insert( HATE_DATA *hatedata, search_tree tree )
 {
     if ( tree == NULL )
     {
       tree = (HATE_NODE * ) malloc( sizeof( HATE_NODE ) );

       if ( tree == NULL )
          bug( "tree_insert: out of space!" );
       else
       {
          tree->hatedata = hatedata;
          tree->left = tree->right = NULL;
       }
     }
     else if ( hatedata->target_char < tree->hatedata->target_char )
       tree->left = tree_insert( hatedata, tree->left );
     else if ( hatedata->target_char > tree->hatedata->target_char )
          tree->right = tree_insert( hatedata, tree->right );

     return tree;
 }

 tree_position tree_find_min( search_tree tree )
 {
    if ( tree == NULL )
       return NULL;
    else if ( tree->left == NULL )
       return tree;
    else
       return tree_find_min( tree->left );
 }

 search_tree tree_delete( HATE_DATA *hatedata, search_tree tree )
 {
    tree_position pos;

    if ( tree == NULL )
       bug( "tree_delete: not found" );
    else if ( hatedata->target_char < tree->hatedata->target_char )
       tree->left = tree_delete( hatedata, tree->left );
    else if ( hatedata->target_char > tree->hatedata->target_char )
         tree->right = tree_delete( hatedata, tree->right );
    else if ( tree->left && tree->right )
    {
       pos = tree_find_min( tree->right );
       tree->hatedata = pos->hatedata;
       tree->right = tree_delete( tree->hatedata, tree->right );
    }
    else
    {
       pos = tree;
       if ( tree->left == NULL )
         tree = tree->right;
       else if ( tree->right == NULL )
         tree = tree->left;
       free( pos );
    }

    return tree;
 }

 HATE_DATA *tree_retrieve( tree_position pos )
 {
    return pos->hatedata;
 }

...

 struct hate_data
 {
    CHAR_DATA *target_char;
    int hate_amount;
 };

 struct hate_node
 {
    HATE_DATA *hatedata;
    search_tree left;
    search_tree right;
 };

...

mob->hatedata = tree_make_empty( NULL );

Example use:

if ( IS_NPC(victim) )
{
     HATE_DATA *hatedata;
   tree_position P;

   if( ( P = tree_find( ch, victim->hatedata )) == NULL || tree_retrieve( P )->target_char != ch )
   {
     int test;
     hatedata = (HATE_DATA * ) malloc( sizeof( HATE_DATA ) );
     hatedata->target_char = ch;
     test = number_range( 1, 50 );
     hatedata->hate_amount = test;
     victim->hatedata = tree_insert( hatedata, victim->hatedata );
     ch_printf( ch, "It should now hate you for %d.\n\r", test );
   }
   else
   {
     hatedata = tree_retrieve(tree_find( ch, victim->hatedata ));
     ch_printf(ch, "You are already hated for %d!\n\r", hatedata->hate_amount );
   }

}

Do you have any suggestions? Does it look okay? Are there any ways to optimize it?

In tree_find and tree_find_min [edit: and even in tree_insert] you're not really gaining anything from using recursion. For example, I think tree_find_min would probably be clearer something like:

tree_position tree_find_min( search_tree tree )
{
   if ( tree == NULL )
      return NULL;
   while (tree->left != NULL)
       tree = tree->left;
   return tree;
}

As a side-benefit, this may also be faster with some compilers. In code like:

 HATE_DATA *hatedata;

 /* ... */

 hatedata = (HATE_DATA * ) malloc( sizeof( HATE_DATA ) );

I'd change it to look more like:

 hatedata = malloc(sizeof(*hatedata));

The cast accomplishes nothing useful in C, and can cover up the bug of forgetting to #include <stdlib.h> to get the proper prototype for malloc. Using sizeof(*hatedata) instead of sizeof(HATE_DATA) means that changing the type only requires changing it in one place (where you've defined the variable), instead of everywhere you've done an allocation.


Getting filtered autolist Is there a better way to write the following code fragment?

I can't have more than 10 conditional statement in my method as it gives cyclomatic complexity.

private List<AutoDto> getFilteredAutoList()
{
    List<AutoDto> autoList = getAutoListForCoverage();
    List<AutoDto> filteredAutoList = new ArrayList<AutoDto>();

    for(AutoDto auto : autoList)
    {
        if("FAQ025".equals((auto.getCarCode()))
                ||"FAQ025".equals((auto.getCarCode()))
                ||"QEF025".equals((auto.getCarCode()))
                ||"QEF037AB".equals((auto.getCarCode()))
                ||"FAQ037AB".equals((auto.getCarCode()))
                ||"FAQ045".equals((auto.getCarCode()))
                ||"QEF045".equals((auto.getCarCode()))
                ||"NBEF028".equals((auto.getCarCode()))
                ||"OPCF028".equals((auto.getCarCode()))
                //20 more OR conditions
                )
        {
            filteredAutoList.add(auto);
        }
    }
    return filteredAutoList;
}

The following two approaches came to my mind:

Using HashSet

private List<AutoDto> getFilteredAutoList()
{
    List<AutoDto> autoList = getAutoListForCoverage();
    List<AutoDto> filteredAutoList = new ArrayList<AutoDto>();

    Set<String> carCodeSet = getCarCodeSet();

    for(AutoDto auto : autoList)
    {
        if(carCodeSet.contains(auto.getCarCode()))
        {
            filteredAutoList.add(auto);
        }
    }
    return filteredAutoList;
}

private Set<String> getCarCodeSet()
{
    Set<String> carCodeSet = new HashSet<String>();

    carCodeSet.add("FAQ025");
    carCodeSet.add("QEF025");
    carCodeSet.add("QEF037AB");
    carCodeSet.add("FAQ037AB");
    carCodeSet.add("FAQ045");
    carCodeSet.add("QEF045");
    carCodeSet.add("NBEF028");
    carCodeSet.add("OPCF028");
    //20 more codes to be added

    return carCodeSet;
}

Using String contains method

private List<AutoDto> getFilteredAutoList1()
{
    List<AutoDto> autoList = getAutoListForCoverage();
    List<AutoDto> filteredAutoList = new ArrayList<AutoDto>();

    String carCodeString = "FAQ025,QEF025,QEF037AB,FAQ037AB,FAQ045,QEF045,NBEF028,OPCF028";
    for(AutoDto auto : autoList)
    {
        if(carCodeString.contains(auto.getCarCode()))
        {
            filteredAutoList.add(auto);
        }
    }
    return filteredAutoList;
}

Which approach should I follow and why?

Why don't you simply use an array of strings, as in:

String carCodeSet[] = {"FAQ025","QEF025","QEF037AB", /*...*/ };

this seems to me cleaner. Of course the HashSet would have better performance if the list of codes is very long... but maybe for 20 items a linear search on the array is not bad.

The String.contains method poses some problems in the fact that you cannot have the separator (comma) in your strings and, more importantly, that any sub-string would match: for example the code "FAQ" would be found as a sub-string. These might not be an issue in your actual use case, but is something which I would prefer not to have in my code.


malloc in main() or malloc in another function: allocating memory for a struct and its members When initializing a struct in C, we can allocate memory inside the main function or within another function and return a pointer to the newly created struct. This first example shows the latter; memory is allocated in Buffer_create and a pointer is returned:

#include <stdio.h>
#include "buffer.h"

int main(int argc, char *argv[])
{
    struct Buffer *tx_buffer = Buffer_create(8);

    Buffer_destroy(tx_buffer);

    return 0;
}

And this one shows how all memory allocations can be done within the main function:

#include <stdio.h>
#include "buffer.h"

int main(int argc, char *argv[])
{
    uint8_t *ptr_rx_buffer = malloc(sizeof(uint8_t)*8);
    struct Buffer *rx_buffer = malloc(sizeof(struct Buffer));
    Buffer2_create(rx_buffer, ptr_rx_buffer, 8);

    Buffer2_destroy(rx_buffer);

    return 0;
}

And here are the contents of the header file buffer.h:

#ifndef _buffer_h
#define _buffer_h

#include <stdint.h>
#include <stdlib.h>

struct Buffer {
    uint8_t *buffer;
    size_t size;
};

struct Buffer *Buffer_create(size_t size);

void Buffer_destroy(struct Buffer *who);

void Buffer2_create(struct Buffer *who, uint8_t *buffer, size_t size);

void Buffer2_destroy(struct Buffer *who);

#endif

And buffer.c:

#include <stdint.h>
#include <assert.h>
#include <stdlib.h>
#include "buffer.h"

struct Buffer *Buffer_create(size_t size)
{
    struct Buffer *who = malloc(sizeof(struct Buffer));
    assert(who != NULL);

    who->buffer = malloc(sizeof(uint8_t)*size);
    who->size = size;

    return who;
}

void Buffer_destroy(struct Buffer *who)
{
    assert(who != NULL);
    free(who->buffer);
    free(who);
}

void Buffer2_create(struct Buffer *who, uint8_t *buffer, size_t size)
{
    assert(who != NULL);

    who->buffer = buffer;
    who->size = size;
}

void Buffer2_destroy(struct Buffer *who)
{
    assert(who != NULL);
    free(who->buffer);
    free(who);
}

The Result

Both approaches work and the executable files for both end up being the same size.

My Question

Will either of these approaches result in memory leaks or poor performance?

Looking at the performance, the two versions should perform just about identically.  The second version has one less call/return, which can save a couple of CPU cycles, but if you have it multiple places in your code the additional code bytes and cache misses can overshadow that.  Either way you probably won't notice a difference.

Looking at readability and maintainability, the first version is much better.  You know at a glance what it is doing (rather than looking at several lines to figure it all out), you won't forget any important steps, and error checking is much easier since most of it can be handled in one place (excepting the last check for successful creation of the buffer).  Debugging can also be easier, since you can set a breakpoint on the creation or destruction functions if necessary.


Processing selected and deselected elements I think that this code is very bad because it does not agree with DRY principle. Can you advise me how to improve it?

public void WhatSelected(object paramaters)
    {
        string ElementToProcess = BASM.FindElement(paramaters, true);

        SeleсtedCurrent.Add(ElementToProcess);

        int i = (from x in AssetSelectorCollection where x.IsChecked == true select x).Count();
        if (i >= state)
        {
            foreach (BinaryAssetSelector item in AssetSelectorCollection)
            {
                if (item.IsChecked == false) { item.IsEnable = false; item.IsChecked = false; }
            }
        }
        if (state != 1)
        {
            foreach (BinaryAssetSelector item in AssetSelectorCollection)
            {
                if (item.IsChecked == true && item.IsEnable == false)
                {
                    item.IsEnable = true;
                }
            }
        }
        Messenger.Default.Send<BinarySelectorCommunicator>(new BinarySelectorCommunicator { SelectedIN = SeleсtedCurrent });
    }

    public void WhatDeSelected(object param)
    {
        string ElementToProcess = BASM.FindElement(param, false);

        SeleсtedCurrent.Remove(ElementToProcess);

        int i = (from x in AssetSelectorCollection where x.IsChecked == true select x).Count();
        if (i <= state)
        {
            foreach (BinaryAssetSelector item in AssetSelectorCollection)
            {
                if (item.IsEnable == false)
                {
                    item.IsEnable = true;
                }
            }
        }
        if (state != 1 && i == 1)
        {
            foreach (BinaryAssetSelector item in AssetSelectorCollection)
            {
                if (item.IsChecked == true)
                {
                    item.IsEnable = false;
                }
            }
        }
        Messenger.Default.Send<BinarySelectorCommunicator>(new BinarySelectorCommunicator { SelectedIN = SeleсtedCurrent });
    }

Variable names:

Names like i, param, parameters, state don't have a useful meaning, not for you, nor for others who are reading/reviewing your code. Use meaningful names for your variables, this is better for readability and maintainability.

You have four different actions, based on the four different conditions regarding state and i:

i >= state
state != 1
i <= state
state != 1 && i == 1

Is there a reason why they are in separate methods? The only overlapping part is that in two conditions i can be the same as state (1 and 3). If this is a mistake, you can place all the code in one method, if the checks are correct and you want to keep both methods, also fine. I'll base my answer on the correctness of your code and keep two methods.

In both the methods you perform two foreach loops based on a check. This means that if AssetSelectorCollection has 200 items, you loop 400 times, which is not smart. Reverse the code like this:

foreach (BinaryAssetSelector item in AssetSelectorCollection)
{
    if (i >= state)
    {
        if (item.IsChecked == false)
        {
            item.IsEnable = false;
            item.IsChecked = false;
        }
    }

    if (state != 1)
    {
        if (item.IsChecked == true && item.IsEnable == false)
        {
            item.IsEnable = true;
        }
    }
}

Now you only have one loop over the collection, which will increase performance, certainly over a bigger collection. This must be applied in both methods.

Redundant checking:

Following code:

if (item.IsChecked == false)
{
    item.IsEnable = false;
    item.IsChecked = false;
}

contains redundant checks. The IsChecked property is a boolean and thus needs no evaluation against true or false in the condition, plus you un-check the item when it is already unchecked, no need to do that twice. Rewrite it as follows:

if (!item.IsChecked)
{
    item.IsEnable = false;
}

and even shorter:

item.IsEnable = item.IsChecked

Note that this last line will enable the item when it is checked, not only disable the item when it is unchecked. This logic can also be applied to all the if statements in the code.

Example of the first method, reviewed:

public void WhatSelected(object paramaters)
{
    string ElementToProcess = BASM.FindElement(paramaters, true);
    SeleсtedCurrent.Add(ElementToProcess);

    var i = (from x in AssetSelectorCollection where x.IsChecked == true select x).Count();

    foreach (BinaryAssetSelector item in AssetSelectorCollection)
    {
        if (i >= state)
        {
            if (!item.IsChecked)
            {
                item.IsEnable = false;
            }
        }

        if (state != 1)
        {
            if (item.IsChecked && !item.IsEnable)
            {
                item.IsEnable = true;
            }
        }
    }
    Messenger.Default.Send<BinarySelectorCommunicator>(new BinarySelectorCommunicator { SelectedIN = SeleсtedCurrent });
}

Edit:

Since you're not doing anything between the two if statements you can place the condition of the second statement inside the first one with an && operator:

public void WhatSelected(object paramaters)
{
    string ElementToProcess = BASM.FindElement(paramaters, true);
    SeleсtedCurrent.Add(ElementToProcess);

    var i = (from x in AssetSelectorCollection where x.IsChecked == true select x).Count();

    foreach (BinaryAssetSelector item in AssetSelectorCollection)
    {
        if (i >= state && !item.IsChecked)
        {
            item.IsEnable = false;
        }

        if (state != 1 && (item.IsChecked && !item.IsEnable))
        {
            item.IsEnable = true;
        }
    }
    Messenger.Default.Send<BinarySelectorCommunicator>(new BinarySelectorCommunicator { SelectedIN = SeleсtedCurrent });
}


Simple single-file PHP login system This is something that I initially came up with to protect a bunch of scripts on my personal webserver, but am now planning on using with a few public projects too.

login.php:

<?php
session_start();

//log out if ?logout=true
if($_GET["logout"]){
    $_SESSION["loggedIn"]=false;
    session_destroy();
    header('Location: '. $_SERVER['SCRIPT_NAME']);
    exit();
}

$password = '8f434346648f6b96df89dda901c5176b10a6d83961dd3c1ac88b59b2dc327aa4';

if(empty($_SESSION["loggedIn"])){
    if(isset($_POST["password"])){
        if(hash('sha256', $_POST['password'])===$password){
                session_regenerate_id(true);
                $_SESSION["loggedIn"]=true;
        } else{
            echo "Incorrect password!
";
        }
    }
}

if($_SESSION["loggedIn"] == true){
print '
    You are logged in!

    Logout

';
} else{
print '
Login

      <form action="#" method="post">
        Password: <input type="password" name="password">

      <input type="submit" value="Login!" />
    </form>
';
}
?>

For other files which should only be accessible if the user is logged in:

OtherFiles.php:

<?php
session_start();
if($_SESSION["loggedIn"]==false){
    session_destroy();
    header('Location: login.php');
    exit();
} else {
    //stuff goes here
}

Yes, I know, sha-256 hashes aren't recommended for password storage, but that's only for testing, when deployed it'll be using proper salting for password storage. What I need feedback on is the login script itself.

Perhaps use some strategies to counter CSRF as well. Take a look at this comprehensive guide: https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet


Check if array A contains all elements of array B and vice versa irrespective of order I am trying to check if an Array A contains all the elements of Array B and similarly Array B contains elements of Array A, the order does not matter.

Some examples:

A               B                Result
{1}             {0}              false
{0, 0, 0}       {0, 0, 0}        true
{17, 4, 11}     {4, 17, 11}      true

I have written the following code:

public static bool CheckEquality(int[] a, int[] b)
{
    return Array.TrueForAll(a, m => b.Count(z => m == z) == a.Count(z => m == z)) && a.Length == b.Length;
}

Both arrays contain just integers. Both arrays should strictly contain the same number of the same elements, and their lengths should be equal.

Is there a better and efficient way to do this?

You can check this by keeping track of each value in a dictionary: adding one for items in the first array, subtracting one for items in the second array and finally checking that all tracked values are zero.

Also, implemented this way, there is no need to make the method array- or int-specific.

public static bool CheckEquality<T>(IEnumerable<T> first, IEnumerable<T> second)
{
    var dictionary = new Dictionary<T, int>();

    Action<IEnumerable<T>, int> setCounts = (items, change) =>
    {
        foreach (var item in items)
        {
            int count;
            // if not found, count will be the default value of 0
            dictionary.TryGetValue(item, out count);
            dictionary[item] = count + change;
        }
    };

    setCounts(first, +1);
    setCounts(second, -1);

    return dictionary.Values.All(value => value == 0);
}

The code is more complicated than sorting arrays, but it's also \$O(n)\$.


Maintaining a count with two threads using conditional variables This is just a program to show the use of conditional variables when two threads are involved. One thread wants a non-zero value of count, and other thread is responsible for signaling it when the count is non-zero.

Is there something in this code which still needs an improvement? Is there something which could have been done in a better way?

#include <iostream>

/* Declaration of a Mutex variable `mutexA`. */
pthread_mutex_t mutexA;

/* Declaration of a Condition Variable `conditionVariableA`. */
pthread_cond_t  conditionVariableA;

/* `functionA` and `functionB` are the argument functions of the two threads (declared
below) */
void* functionA (void*);
void* functionB (void*);

/* `count` is the variable shared between threads `A` and `B`.
Thread `A` wants it to be non zero. Thread `B` will be responsible for making it
non-zero and then issuing a signal. */
int  count = -100;

int main ()
{
    // Declaration of two threads namely `A`, and `B`.
    pthread_t A, B;

    // Initializing the mutex lock to be shared between the threads.
    pthread_mutex_init (&mutexA, NULL);

    /* The function `pthread_cond_init()` initialises the Condition Variable referenced by
    variable `conditionVariableA` with attributes referenced by variable `attributes`. If
    `attributes` is NULL, the default condition variable attributes are used. */
    pthread_cond_init (&conditionVariableA, NULL);

    /* Definition of two threads namely A and B */
    pthread_create (&A, NULL, functionA, NULL);
    pthread_create (&B, NULL, functionB, NULL);

    pthread_join (A, NULL);
    pthread_join (B, NULL);
}

void* functionA (void* argA)
{
    while (1)
    {
        pthread_mutex_lock (&mutexA);

        if (count <= 0)
        {
            std :: cout << "\ngnitiaW!\n";
            pthread_cond_wait (&conditionVariableA, &mutexA);
        }
        else
        {
            // Do something.
            std :: cout << "\nTime to enjoy!\n";
            return 0;
        }
        pthread_mutex_unlock (&mutexA);
    }
    return 0;
}

void* functionB (void* argB)
{
    while (1)
    {
        pthread_mutex_lock  (&mutexA);

        count++;

        if (count > 0)
        {
            pthread_cond_signal (&conditionVariableA);
            std :: cout << "\nSignaled!\n";
            return 0;
        }
        else
        {
            std :: cout << "\nNot signaled yet!";
        }

        pthread_mutex_unlock (&mutexA);
    }
    return 0;
}

Couple of issues:

The code is not exception safe.
Any resource that has an open/close symantic should be handled via RAII

{
    pthread_mutex_lock (&mutexA);

    // Code
    // This may throw an exception thus will
    // cause the code to miss the unlock.

    pthread_mutex_unlock (&mutexA);
}

To fix this you should us an RAII locker object.

{
    MutexLocker    lock(mutexA);  // constructor calls lock.
                                  // detructor calls unlock.
    // Code
    // This may throw an exception
    // and it still works correctly.
}

It may work in this simple example:

    if (count <= 0)
    {
        std :: cout << "\ngnitiaW!\n";
        pthread_cond_wait (&conditionVariableA, &mutexA);
    }

But normally you need to place the wait() inside a loop.

    while (count <= 0)
    {
        std :: cout << "\ngnitiaW!\n";
        pthread_cond_wait (&conditionVariableA, &mutexA);
    }

If there are multiple threads that could enter this function then between the signal from the producer thread and this thread waking up from the wait another thread could have stolen the object. Thus you need to recheck the state you were waiting for and go back to sleep if it was stolen.

This would have been fixed by the MutexLocker above. But the code as it stands leaves the lock locked.

    else
    {
        // Do something.
        std :: cout << "\nTime to enjoy!\n";

        // You are  returning early without unlock the lock.
        // This causes all your other threads to stall
        // and probably causes deadlock.
        return 0;
    }

    // This point is not reached if you return early.
    pthread_mutex_unlock (&mutexA);