9
\$\begingroup\$

I can't wrap my head around that. There seem to be a lot of redundancy, and even if it is not called called more than ten times in the main loop, I still find it hard to swallow that it loops through the whole list at each invocation.

def get_file_info(f, stream, field):

    discoverer = GstPbutils.Discoverer()
    discoverer.connect('discovered', on_discovered)
    info = discoverer.discover_uri(f)

    for vinfo in info.get_video_streams():
        ftype = (vinfo.get_stream_type_nick())
        width = vinfo.get_width()
        height = (format(vinfo.get_height()))
        depth = (format(vinfo.get_depth()))
        framerate = ('Frate: {}'.format(vinfo.get_framerate_num()))
        bitrate = ('Brate: {}'.format(vinfo.get_max_bitrate()))
        par = vinfo.get_par_num()
        fname = vinfo.get_caps().to_string().split(',')[0]

    if stream == 'video':
        if field == 'duration':
            return str(convert_time(info.get_duration(), 'hh:mm:ss'))
        elif field == 'width':
            return str(width)
        elif field == 'height':
            return str(height)
        elif field == 'type':
            return str(ftype)
        elif field == 'depth':
            return str(depth)
        elif field == 'framerate':
            return str(framerate)
        elif field == 'bitrate':
            return str(bitrate)
        elif field == 'par':
            return str(par)
        elif field == 'name':
            return str(par)

I read the lambda documentation, but can't seem to write a proper one, but I would sure like to.

\$\endgroup\$
  • \$\begingroup\$ Are the fields either "duration", or "width" or .... "name", or can it contain any string? \$\endgroup\$ – Stewie Griffin Jun 29 '16 at 7:35
  • \$\begingroup\$ Yes! they are just a fixed set of no more than the nine listed here ; No, it can not contain any string. \$\endgroup\$ – yPhil Jun 29 '16 at 7:41
  • 3
    \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ – BCdotWEB Jun 29 '16 at 8:30
22
\$\begingroup\$

@Stewie Griffin's answer is good, but I dare say it promotes eval, where dict is safer and more expansive.
The main aim here is to use the same if's as Stewie, as they are definitely good. As a dictionary contains things and you can't get other things out of it it's safer. With Stiewies for example you can pass 'get_file_info' as the field and it'll give you '<function get_file_info at 0x03335150>'. It also means that you can return a dictionary for each and every stream in info.get_video_streams().

To obtain this we will make a variable video_info that will hold the needed information.

video_info = {
    'width': 2,
    'height': 3,
    'name': 'Name of video'
}

field = 'width'
str(video_info[field])
Out[3]: '2'

field = 'name'
str(video_info[field])
Out[5]: 'Name of video'

This means we just need to move all the information into this dictionary. And so you'll need to change the for-loop to create a dictionary.

video_info = {
    'type': vinfo.get_stream_type_nick(),
    'width': vinfo.get_width(),
    'height': format(vinfo.get_height()),
    'depth': format(vinfo.get_depth()),
    'framerate': 'Frate: {}'.format(vinfo.get_framerate_num()),
    'bitrate': 'Brate: {}'.format(vinfo.get_max_bitrate()),
    'par': vinfo.get_par_num(),
    'fname': vinfo.get_caps().to_string().split(',')[0]
}

I'd also go on to say that you can change the code to return a dictionary and you can then remove all the field stuff from this function. And you can use a list comprehension to return a dictionary for every info.get_video_streams().

which leads to:

def get_file_info(f, stream):
    discoverer = GstPbutils.Discoverer()
    discoverer.connect('discovered', on_discovered)
    info = discoverer.discover_uri(f)
    duration = convert_time(info.get_duration(), 'hh:mm:ss')
    return [
        {
            'duration': duration,
            'type': vinfo.get_stream_type_nick(),
            'width': vinfo.get_width(),
            'height': format(vinfo.get_height()),
            'depth': format(vinfo.get_depth()),
            'framerate': 'Frate: {}'.format(vinfo.get_framerate_num()),
            'bitrate': 'Brate: {}'.format(vinfo.get_max_bitrate()),
            'par': vinfo.get_par_num(),
            'fname': vinfo.get_caps().to_string().split(',')[0],
        }
        for vinfo in info.get_video_streams()
    ]

def get_file_field(f, stream, field):
    videos = get_file_info(f, stream)
    video_info = videos[-1]
    if stream == 'video':
        return str(video_info[field])
\$\endgroup\$
  • \$\begingroup\$ I'm confused by the get_file_info return code. You are returning a list that contains a dict (I think commas missing between key:value pairs), and then a for loop? I've never seen anything like this \$\endgroup\$ – joel goldstick Jun 29 '16 at 9:52
  • \$\begingroup\$ @joelgoldstick Thank you, I forgot to add commas in my dictionary, ): I'm using a list comprehension to return a list. It's like map but simpler to write. I can't really find anything that explains it too well, but these may help, and it's a way to remove the l=[];for a in ...: l.append(a); return l idiom. \$\endgroup\$ – Peilonrayz Jun 29 '16 at 10:24
  • \$\begingroup\$ This is beautiful :) \$\endgroup\$ – yPhil Jun 29 '16 at 19:10
5
\$\begingroup\$

As far as I can tell, the first part of you code looks fine.

However, all the elif calls can be avoided. The first if is necessary, since you're calling convert_time. The others can be simplified, if you know the fields are one of the 9 you are listing. Instead of checking if the names are similar, and then call str(string), you can use the field name directly, in combination with locals().

I believe this is a better way to do it:

if stream == 'video':
       if field == 'duration':
           return str(convert_time(info.get_duration(), 'hh:mm:ss'))
       else
           return str(locals()[field])

locals()[field] will return whatever field contains. Then, str(locals()[field])` will convert it to a string.

width = 2    
height = 3    
name = 'Name of video'

field = 'width'    
str(locals()[field])
Out[5]: '2'

field = 'name'    
str(locals()[field])
Out[7]: 'Name of video'

Note, I'm not the most experienced Python user, so it's possible that there are better ways to do this.

\$\endgroup\$
  • 11
    \$\begingroup\$ A safer alternate to this is to use dict. eval is ok, but may lead to bad or unsafe habits. \$\endgroup\$ – Peilonrayz Jun 29 '16 at 8:25
  • 2
    \$\begingroup\$ Thanks @JoeWallis =) I didn't know it could be used here. I'm quite sure both me, OP and others would be happy to see a review that explains how to solve this in a better, safer way =) I'll look into how dict can be used here when I have the time =) \$\endgroup\$ – Stewie Griffin Jun 29 '16 at 8:32
  • 1
    \$\begingroup\$ @StewieGriffin just replace eval(field) with locals()[field] \$\endgroup\$ – jacg Jun 29 '16 at 8:56
  • \$\begingroup\$ @StewieGriffin or use a pre-meditated explicit dict for this purpose in the for-loop: d = {'ftype': (vinfo.get_stream_type_nick()), 'width': vinfo.get_width(), ...} and then you can use d[field] instead of eval(field) \$\endgroup\$ – jacg Jun 29 '16 at 9:01
  • 2
    \$\begingroup\$ But using locals you could query fields not intended for that use, like discoverer. Using a dictionary, like Joe suggested is much better idea. \$\endgroup\$ – CodesInChaos Jun 29 '16 at 15:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.